Doist / reactist

Open source React components made with ❤️ by Doist
http://doist.github.io/reactist
MIT License
248 stars 22 forks source link

feat: support dismissing toast when action is clicked #763

Closed pedroalves0 closed 1 year ago

pedroalves0 commented 1 year ago

Short description

Motivated by an internal feature (https://github.com/Doist/todoist-web/pull/6438#discussion_r1171686224), it's a reasonable assumption that when triggering the toast action the toast should be dismissed. Here we are adding support for it by passing an onDismiss function to the action handler, leaving the responsibility to call it to the latter. I kept the scope of the changes contained to non-custom actions. If the need arises for custom actions, we can expand the implementation later on.

PR Checklist

Versioning

Minor

gnapse commented 1 year ago

Thanks for this contribution, Pedro. It does seem like an oversight on the initial API.

I would go a step further and ask: why not make this the default automatic behavior? That is, making it so that clicking on an action button in the toast will always dismiss the toast, without having to pass onDismiss into the action click handler. I think that that would be a cleaner API, and a reasonable behavior. Thoughts?

pedroalves0 commented 1 year ago

A certain degree of customization is lost compared to my initial proposal, but it's hard to think of a use case where an action doesn't lead to the dismissal of a toast, so I have nothing against doing it, @gnapse. I pushed the changes in https://github.com/Doist/reactist/pull/763/commits/0f87e727fa8cb6ee2ac0d883e47cd1a80873aae1. Note that I purposely let the StaticToast API untouched so that there's a clear differentiation between a dismiss and a custom action, and delegated the simultaneous action and dismissal calls to the useToasts domain.

pedroalves0 commented 1 year ago

Remember to update the CHANGELOG. Also, are you planning to release this? Or just merge it so it becomes available in the next release?

Normally I'd create the release in a separate commit directly on main, after merging the PR, but I've done it now in https://github.com/Doist/reactist/pull/763/commits/9292cc4bed1401ddfd475c2dc9a1329332668b0d.