Added persistent state for notifications and made default timeout 2.5s #82
Conversation
| `; | ||
|
|
||
| export const Notification = ({ message, live, children, variant, timeout, onClose, ...props }) => { | ||
| export const Notification = ({ message, live, children, variant, timeout, onClose, persistent, ...props }) => { |
There was a problem hiding this comment.
do you think the persistent prop collides with the timeout prop? for example:
<Notification timeout={5000} persistent>
...
</Notification>looking through the code it seems like the example will be persistent, but is that clear from where we are using the component? we could avoid this by saying timeout={null | undefined | 0} means persistent, but i'm not sure that's very clear either!
There was a problem hiding this comment.
yeah this is something I struggled with - the previous definition has timeout = 0 as persistent, but that didn't feel great to me. I can add a custom function to the persistent PropType which throws an error if persistent === true and timeout > 0; what do you think?
There was a problem hiding this comment.
do you think a notification would ever be conditionally persistent? if we threw an error, then something like <Notification persistent={shouldBePersistent} timeout={5000} /> would need to be <Notification persistent={shouldBePersistent} timeout={shouldBePersistent === false ? 5000 : 0} /> so that we don't pass in something invalid.
maybe a wild idea: what if it was something like timeout="never" to disable the timeout?
pheebcodes
left a comment
pheebcodes
left a comment
There was a problem hiding this comment.
one somewhat philosophical comment!
pheebcodes
left a comment
pheebcodes
left a comment
There was a problem hiding this comment.
approved! we're good with the persistent prop.
discussion in slack decided against throwing an error if persistent == true && timeout !== 0 since timeout defaults to 2500. it wouldn't be great to have to explicitly set timeout={0} when persistent={true} to avoid the error. also the prop is documented so it's easy to find that persistent overrides timeout without digging into the shared-component code.
sheridanvk
changed the base branch from
sheridan/ch8571/add-ability-to-remove-notifications
to
master
|
🚀 PR was released in |
4 participants
In general, the notifications that we show up disappear after 2.5s. This makes that the default, and adds a persistent state in the cases where we don't want them to disappear.
Worth noting that in the Community site, there's an inline Notification that is usually persistent. I think we should handle this case by creating a separate InlineNotification component which is used outside of NotificationsProvider, that sets the Notification to persistent.