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 Inline #1332

Merged
merged 11 commits into from
Jan 25, 2022
Merged

Add Notification Inline #1332

merged 11 commits into from
Jan 25, 2022

Conversation

amelako
Copy link
Contributor

@amelako amelako commented Dec 24, 2021

Addresses #792.

Purpose

Introduces the new NotificationInline component.
The Notification Inline component non-disruptively provides quick and contextual inline notification and it is integrated within the content section.
Screenshot 2022-01-14 at 15 08 07

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 and an action button.
Inline notifications are part of the content and should be placed near their related part of the content.
Inline notifications can be dismissed by clicking on a close button in the top-right corner.

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 24, 2021

🦋 Changeset detected

Latest commit: f771c33

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

@sumup-clark
Copy link

sumup-clark bot commented Dec 24, 2021

Hey @amelako,
we are super excited that you are contributing! 🎉There's one more thing you need to do. Please accept our Contributor License Agreement. It helps you and us to collaborate on clear terms and focus on what we love most: code.

Thanks!

@vercel
Copy link

vercel bot commented Dec 24, 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/bhVhkXNAtvVck9yNAqFhaDtY613A
✅ Preview: https://oss-circuit-ui-git-notification-inline-sumup.vercel.app

@codecov
Copy link

codecov bot commented Dec 24, 2021

Codecov Report

Merging #1332 (f771c33) into main (bec9460) will decrease coverage by 0.02%.
The diff coverage is 92.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1332      +/-   ##
==========================================
- Coverage   92.23%   92.20%   -0.03%     
==========================================
  Files         190      191       +1     
  Lines        3787     3838      +51     
  Branches     1209     1224      +15     
==========================================
+ Hits         3493     3539      +46     
- Misses        275      279       +4     
- Partials       19       20       +1     
Impacted Files Coverage Δ
...components/NotificationToast/NotificationToast.tsx 88.88% <ø> (ø)
...mponents/NotificationInline/NotificationInline.tsx 92.15% <92.15%> (ø)
...mponents/NotificationBanner/NotificationBanner.tsx 81.96% <0.00%> (-1.64%) ⬇️

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.

I like the direction this is going!

Added a few general comments on the component and the docs for now, I'll also re-review later.

For docs I'm kind of missing something that explains what the difference between the toast and this component is, in terms of behavior, a11y etc. They're pretty similar and I expect that some developers will find it confusing. Could we add this somewhere (in the intro maybe)?

}
| { onClose?: never; closeButtonLabel?: never };

export type BaseProps = HTMLAttributes<HTMLDivElement> & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we share some of these props between the toast and this component? There should be few differences

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.

Oops I submitted the previous review by mistake and didn't have time to finish writing my thoughts down

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.

Just a few extra thoughts about docs, thanks for addressing my previous review's more general comments 🙌

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.

Nice! Let's release this and I might add a bit more to the docs separately 👀 The new component looks great!

.changeset/nine-tigers-approve.md Outdated Show resolved Hide resolved
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