-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[feature]: Toasts #1218
[feature]: Toasts #1218
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://venia-git-feature-toasts.magento-research1.now.sh |
packages/venia-concept/src/components/OnlineIndicator/onlineIndicator.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sirugh This looks much better. There are still some rough edges, but this is a big improvement over what we have. A few questions & concerns I still have:
- Is it possible to add a user-dismissable toast that doesn't get auto-dismissed? If
timeout
is0
or falsy at all, it should probably not set a timeout, right? - If a toast has a timeout, and a user tries to copy some text from it to their clipboard, won't the toast often disappear before they can do so? In most apps, I think a toast's timeout is cleared when the user interacts with that toast at all.
- The
icon
prop of toasts is too tightly coupled to our visual presentation of toasts, I think. I'd prefer to use thetype
enum to determine what icons to render; if end users want to change the presentation, they'll override theToast
component or use a hook version of it from Peregrine. As is,icon
would effectively be a render prop, which we're avoiding. Also, we probably don't want to store functions in state, even if this isn't Redux.
@soumya-ashok and I discussed this, though not sure if we documented it anywhere. Basically all toasts should auto-dismiss for now and the answer for user-dismissable toasts would be to set some long timeout. I can add a workaround to this that does not set the timeout if
This is a good point. We would need some mechanic to pause the timeout. react-toast-notifications does this and we could too. If you feel strongly about this I can do some work to get this functionality in, otherwise I would say that we hardcode some longer
The way we create/use icons in our application is that we have the component who needs it import/use it. We can't know all the different icons that toast emitters want to put into their implementations, so we allow them to pass the whole icon. This is definitely a tight coupling to the display, but intentionally so that the display doesn't have to import every possible icon. We could have the toast component enumerate a subset of icons and basically ask anyone who reimplements Toasts to provide an icon enums as well. Again, we can talk about this before I make changes. |
I like this approach.
I also like this approach.
This is exactly what I'd like to do. We already have the enum with |
Can you point me at the |
Here.
|
Oh so the type of the toast does not indicate the icon, unfortunately. Anyone could emit a sad face icon with an error toast to indicate some error or a cloud-off icon with an error toast to indicate offline. |
…matically by passing 0 or false as timeout prop
Created #1284 for timeout pausing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sirugh Thanks for making changes. I've updated the prop types of Toast
. Ready to merge it?
TODO LIST
CSS Transitions for toast addition and removalDecided that pop-in/out was fine.Description
This PR brings you 🥁 🥁 🥁 🥁 🥁 🥁 toasts 🍞 !
Big changes:
Toasts should:
timeout
, or some default (5 seconds).Related Issue
Closes #783.
Verification Steps
miniCart.js
and rerender. See an error toast!How Have YOU Tested this?
Manually, and with verification steps, and wrote unit tests.
Proposed Labels for Change Type/Package
Checklist: