Skip to content
This repository was archived by the owner on Jan 22, 2026. It is now read-only.

Added persistent state for notifications and made default timeout 2.5s #82

Merged
sheridanvk merged 1 commit into from
Feb 5, 2020
Merged

Added persistent state for notifications and made default timeout 2.5s #82

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions lib/notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const NotificationBase = styled.aside`
animation-timing-function: ease-out;

${({ variant }) => variants[variant]}
${({ closing, timeout }) => {
${({ closing, timeout, persistent }) => {
if (closing) {
return css`
animation-name: ${closingAnimation};
Expand All @@ -66,7 +66,7 @@ export const NotificationBase = styled.aside`
}
`;
}
if (timeout > 0) {
if (!persistent) {
return css`
animation-name: ${closingAnimation};
animation-delay: ${timeout}ms;
Expand All @@ -88,7 +88,7 @@ const NotificationContent = styled.div`
margin-right: var(--space-1);
`;

export const Notification = ({ message, live, children, variant, timeout, onClose, ...props }) => {
export const Notification = ({ message, live, children, variant, timeout, onClose, persistent, ...props }) => {
Copy link

@pheebcodes pheebcodes Feb 5, 2020

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@sheridanvk sheridanvk Feb 5, 2020

Choose a reason for hiding this comment

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

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?

Copy link

@pheebcodes pheebcodes Feb 5, 2020

Choose a reason for hiding this comment

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

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?

const ref = React.useRef();
const [closing, setClosing] = React.useState(false);

Expand All @@ -105,6 +105,7 @@ export const Notification = ({ message, live, children, variant, timeout, onClos
closing={closing}
timeout={timeout}
onAnimationEnd={handleAnimationEnd}
persistent={persistent}
{...props}
>
<NotificationContent>{children || message}</NotificationContent>
Expand All @@ -124,12 +125,13 @@ Notification.propTypes = {
variant: PropTypes.oneOf(Object.keys(variants)),
timeout: PropTypes.number,
onClose: PropTypes.func,
persistent: PropTypes.bool,
};

Notification.defaultProps = {
variant: 'notice',
live: 'polite',
timeout: 0,
timeout: 2500,
};

export const StoryNotification = () => (
Expand All @@ -145,12 +147,16 @@ export const StoryNotification = () => (
<Prop name="variant">The style of notification: "notice", "success", "error", "onboarding" (default "notice").</Prop>
<Prop name="timeout">
The time in milliseconds before the notification automatically fades out and calls its "onClose" prop. If no value is provided, the
notification will not automatically close.
notification will close after the default period specified in the default props.
</Prop>
<Prop name="onClose">
A callback function, called when the close button is clicked or the notification times out. This props is provided by the{' '}
<code>createNotification</code> callback. If no value is provided, the notification will not render a close button.
</Prop>
<Prop name="persistent">
A boolean value. If true, any timeout value is ignored and the Notification will remain until removed either programmatically or by a
user action. If no value is provided, the Notification will be removed after the timeout period.
</Prop>
</PropsDefinition>
<p></p>
</>
Expand Down Expand Up @@ -190,7 +196,7 @@ export const NotificationsProvider = ({ children, ...props }) => {
};

const createErrorNotification = (message = 'Something went wrong. Try refreshing?') => {
return createNotification((props) => <Notification variant="error" timeout={2500} live="polite" message={message} {...props} />);
return createNotification((props) => <Notification variant="error" live="polite" message={message} {...props} />);
};

return { createNotification, createErrorNotification, removeNotification };
Expand Down Expand Up @@ -251,7 +257,7 @@ const UploadNotification = ({ onClose }) => {
}

return (
<Notification timeout={2500} onClose={onClose} message="Uploaded!">
<Notification onClose={onClose} message="Uploaded!">
<Progress value={value} max={100} style={{ display: 'inline-block', width: '4rem' }}>
{value}%
</Progress>
Expand All @@ -278,7 +284,7 @@ const Content = () => {
<Button
onClick={() => {
const notification = createNotification((props) => (
<Notification variant="success" {...props} message="Added power-passenger to collection linen-collection">
<Notification variant="success" {...props} persistent message="Added power-passenger to collection linen-collection">
<p style={{ marginTop: 0 }}>Added power-passenger to collection linen-collection</p>
<Button as="a" variant="secondary" size="tiny" href="https://glitch.com/@modernserf/linen-collection" rel="noopener noreferrer">
Take me there
Expand All @@ -303,7 +309,7 @@ const Content = () => {
<Button
onClick={() => {
const notification = createNotification((props) => (
<Notification variant="onboarding" {...props} message="Create an account to keep this project and edit it anywhere.">
<Notification variant="onboarding" persistent {...props} message="Create an account to keep this project and edit it anywhere.">
<p style={{ marginTop: 0 }}>
<strong>Create an account</strong> to keep this project, and edit it anywhere.
</p>
Expand Down