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

Add notification toast #1295

Merged
merged 11 commits into from
Dec 23, 2021
Merged

Add notification toast #1295

merged 11 commits into from
Dec 23, 2021

Conversation

connor-baer
Copy link
Member

@connor-baer connor-baer commented Dec 1, 2021

Addresses #792.

Purpose

Introduces the new NotificationToast component.
The notification toast component non-disruptively provides quick, contextual or feedback based on the outcome of an action.
Screenshot 2021-12-07 at 16 33 16

It comes in 4 variants, "info", "confirm", "notify" and "alert".
To communicate a message always provide a body copy, and if needed an optional headline can be included above the body copy.
The notification toast floats above the content, positioned at the bottom of the screen, centralized horizontally withing the content area.
The auto-dismiss duration is adjustable (default minimum duration is 3000 ms).
Toasts can also be dismissed by clicking on a close button in the top-right corner. You need to provide closeButtonLabel.

Approach and changes

Refer to the Storybook for further implementation and usage guidelines, and to the threads below for more details and discussion on the implementation.

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented Dec 1, 2021

🦋 Changeset detected

Latest commit: 8341e46

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Dec 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/H111VsbR1A825HtetHksevYwJeUS
✅ Preview: https://oss-circuit-ui-git-notification-toast-sumup.vercel.app

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #1295 (8341e46) into main (a063b20) will decrease coverage by 0.02%.
The diff coverage is 92.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1295      +/-   ##
==========================================
- Coverage   92.25%   92.22%   -0.03%     
==========================================
  Files         186      189       +3     
  Lines        3681     3770      +89     
  Branches     1153     1170      +17     
==========================================
+ Hits         3396     3477      +81     
- Misses        268      275       +7     
- Partials       17       18       +1     
Impacted Files Coverage Δ
packages/circuit-ui/hooks/useStack/useStack.ts 93.10% <ø> (ø)
...components/NotificationToast/NotificationToast.tsx 90.47% <90.47%> (ø)
...ircuit-ui/components/ToastContext/ToastContext.tsx 92.10% <92.10%> (ø)
...rcuit-ui/components/ToastContext/createUseToast.ts 100.00% <100.00%> (ø)
...mponents/NotificationBanner/NotificationBanner.tsx 81.96% <0.00%> (-1.64%) ⬇️

Copy link
Member Author

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

I'm so excited to see this component coming to life. It's very satisfying to see so many existing APIs being reused and seamlessly composed together 🤩

A good chunk of the comments is grammar or typo fixes. Next time, please run documentation or other prose text through Grammarly before requesting reviews on a PR.

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Since we paired on the actual feature, and @connor-baer already gave a thorough review, I'm just adding high-level comments here on things we missed (especially related to accessibility).

Also for docs: I think we should make sure that the "Usage in code" section you've added is more prominent: right now the docs start with props, as usual, but several of these props are actually internal. I'm not sure if there's anything we can do to make this clearer (can we show just props that the useToast takes?) but at least let's emphasise that NotificationToast should never be used directly anyways

amelako and others added 8 commits December 21, 2021 16:23
Initially the component imitated the Modal. This commit tailors the logic to the context of toasts: the hook doesn't expose a removeToast, the provider doesn't accept initialToasts (use useEffect instead), toasts are dismissed automatically after a default 3000ms, etc.
Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Left lots of smaller comments, but it's looking good! 🙂

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Thanks so much, this looks great! Let's move forward and address the minor follow-ups we talked about early in 2022 🥳

@amelako amelako merged commit 3861db2 into main Dec 23, 2021
@amelako amelako deleted the notification-toast branch December 23, 2021 09:28
@github-actions github-actions bot mentioned this pull request Dec 23, 2021
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.

3 participants