Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

carbon-components-react/ToastNotification: Add onClose prop #7859

Closed
1 task done
kubijo opened this issue Feb 22, 2021 · 11 comments · Fixed by #7918
Closed
1 task done

carbon-components-react/ToastNotification: Add onClose prop #7859

kubijo opened this issue Feb 22, 2021 · 11 comments · Fixed by #7918

Comments

@kubijo
Copy link
Contributor

kubijo commented Feb 22, 2021

What package(s) are you using?

  • carbon-components

Detailed description

Will you please add an onClose: () => void prop to the ToastNotification react component?

It should replace the inner setState so that it's possible to implement the animation behaviour defined in your design guidelines without ugly hacks that hijack the close button handler.

I've solved this for now by removing the close button & hooking our handler to onClick, but our UX designer is not amused and would like to keep the close button instead.

@emyarod
Copy link
Member

emyarod commented Feb 23, 2021

@kubijo does the existing onCloseButtonClick help with this use case?

@kubijo
Copy link
Contributor Author

kubijo commented Feb 23, 2021

I wouldn't ask if it did… It's called after internal state setter, which removes the DOM content.

What I do is imperative animation of DOM nodes through gsap & removal in a completion callback, so I need a complete control over what's happening.

@emyarod
Copy link
Member

emyarod commented Feb 23, 2021

I'm not sure if we want to provide a controlled mode for the notification components (i.e. an open or visible prop), but in any case would it be more helpful to just provide the animations with the component out of the box?

@kubijo
Copy link
Contributor Author

kubijo commented Feb 23, 2021

I don't believe that you'd be able to achieve what we're going for just in the ToastNotification component & with just CSS and simple JS, so no… you'd just give me more work to sidestep what you've done internally.

From what I've seen so far this kind of thing seems to be intentionally left out from the library & I sort of believe that it's out of its scope…

On the other hand, my proposition takes minimal amount of work on your side and pretty much unblocks users from doing what they need.

@emyarod
Copy link
Member

emyarod commented Feb 23, 2021

It should replace the inner setState so that it's possible to implement the animation behaviour defined in your design guidelines without ugly hacks that hijack the close button handler.

I'm not sure if our animations for notification dismissal have been finalized, can you share what you may be referencing? but regardless I'm not sure if we'll support custom animations for notification dismissal even after we implement our own animations out of the box

@kubijo
Copy link
Contributor Author

kubijo commented Feb 24, 2021

can you share what you may be referencing?

https://www.carbondesignsystem.com/components/notification/usage
image

This is the part that our designer shared with me… we did change it a bit (as you can see on the screencast) so that the slide-out is to the right.

I'm not sure if we'll support custom animations for notification dismissal even after we implement our own animations out of the box

I can see that being something you wouldn't want to do, but I'm not asking you to… all I need is to be able to do it myself while being hooked onto the close button without hacks involving refs & listeners replacement.
What I need is in the end much the same thing you already have for Modal, so if consistency is the concern here, you'd be more consistent with that.
If I then want different animation, I'll just render the component always open, animate it as I please & stop rendering it after completion without any cooperation with internal logic.

@emyarod
Copy link
Member

emyarod commented Feb 24, 2021

I think it would be great for us to provide the animation out of the box rather than supporting a controlled mode for the notification component. If for whatever reason we decide not to provide the animations out of the box then I will implement a controlled mode (along the lines of AccordionItem, Modal, etc). I reached out to our designers for more details on the notification dismissal animation behavior, and we may end up baking it into the component. does that sound reasonable?

that being said I'll do some exploration on controlled notifications separately and may end up implementing it to provide flexibility

@emyarod
Copy link
Member

emyarod commented Feb 26, 2021

FYI @kubijo #7918

@kubijo
Copy link
Contributor Author

kubijo commented Feb 26, 2021

Thank you 👍

@johnbister
Copy link
Contributor

@emyarod @kubijo it sounds like you worked out a solution. If you think it’s worth it to do out of the box animations for toast notifications in the future, please let me know. I’d love to help on that effort.

@kubijo
Copy link
Contributor Author

kubijo commented Mar 3, 2021

@emyarod @kubijo it sounds like you worked out a solution. If you think it’s worth it to do out of the box animations for toast notifications in the future, please let me know. I’d love to help on that effort.

Considered all I've said above I'm fine with the way things are now (once the onClose gets released that is)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants