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

feat(Notification): add onClose support #7918

Merged
merged 8 commits into from
Mar 24, 2021

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Feb 26, 2021

Closes #7859

This PR adds support for the onClose method on ToastNotification and InlineNotification. Similar to the ComposedModal the default close behavior can be avoided if the method returns false

Changelog

New

  • onClose support in ToastNotification and InlineNotification

Testing / Reviewing

Confirm that the onClose action appears in the storybook action logger. When editing the return value of the method locally, returning false should prevent the notification from closing

@netlify
Copy link

netlify bot commented Feb 26, 2021

Deploy preview for carbon-elements ready!

Built with commit 741f30e

https://deploy-preview-7918--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 26, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 741f30e

https://deploy-preview-7918--carbon-components-react.netlify.app

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 ✅

@@ -266,25 +267,33 @@ export function ToastNotification({
[`${prefix}--toast-notification--${kind}`]: kind,
});

const handleClose = useCallback(
(evt) => {
if (!onClose || onClose(evt) !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the onClose(evt) !== false branch, would we more so want people to be able to configure visibility of the close button if they aren't able to dismiss/close the notification?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for the visibility of the notification itself, similar to how the visibility of modals is controlled rather than the visibility of the modal close button

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emyarod right, the way I interpreted it was that if the onClose prop is called and returns false then it will prevent the close from happening. Is that correct?

If so, then I was just asking that if the notification is unable to be dismissed (the onClose prop would return false) it seems like there would need to be some indicator of that. I think from my comment that was hiding the button if someone couldn't dismiss the notification but it could be something else, for sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this be left up to the consumer to resolve? similar to how we provide this API in our modals

Base automatically changed from master to main March 8, 2021 16:35
@emyarod emyarod requested a review from joshblack March 15, 2021 14:59
@kubijo
Copy link
Contributor

kubijo commented Mar 19, 2021

Hi. I've just upgraded to [email protected] and this doesn't seem to be included. Do you have a clue which release might get this?

Edit... I might have misread the state of affaires… I thought it's already merged, but now I see that it's just approved… the question holds though :-)

@tw15egan
Copy link
Member

@emyarod is this good to go?

@kodiakhq kodiakhq bot merged commit 8f3c2a1 into carbon-design-system:main Mar 24, 2021
@emyarod emyarod deleted the 7859-notification-onclose branch March 25, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

carbon-components-react/ToastNotification: Add onClose prop
4 participants