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

UI: Add close button to version update notification #12320

Conversation

Tomastomaslol
Copy link
Member

Issue: #12281

What I did

  • Added state that contains ids of dismissed notification
  • Added new button to that dismisses notifications on click

Kapture 2020-08-30 at 20 07 47

@shilman
Copy link
Member

shilman commented Aug 30, 2020

@Tomastomaslol this looks awesome! @domyen @ndelangen can you take a look?

@shilman shilman changed the title Add the ability to dismiss notifications by clicking a close button UI: Add close button to version update notification Aug 30, 2020
@domyen
Copy link
Member

domyen commented Sep 2, 2020

I'll design a dedicated UI for this and future notifications. We didn't get to that when this feature was first launched. Hang tight!

@domyen
Copy link
Member

domyen commented Sep 9, 2020

@shilman @Tomastomaslol 👋

Strawman flash notifications UI strawman
The original notification UI was created for Storybook 5.0 to provide a rudimentary upgrade experience for Storybook users. Fast forward a year and we're encountering rough edges in the UX. My goal here is to design a flash notification UI that:

  • Covers more use cases and is more expressive
  • Includes a space for CTAs (dismiss, view, etc)

image

What do you think?

@Tomastomaslol
Copy link
Member Author

@domyen Looks great! 🙂

Should we use a pre-defined list of CTAs? like: 'Close' | 'Open' | 'Dismiss'

Or should the UI be flexible enough to support very long CTAs? like: If you would like to close the notification you can click this link. thank you!

Are you able to share your designs through Zeplin or a similar tool that would allow me to extract the exact CSS properties from your designs?

Have a good day!

@domyen
Copy link
Member

domyen commented Sep 10, 2020

Updated the design to simplify the UI more.

  • No custom action buttons (that was confusing and opens the door to multiple actions in one notification, an over optimization for now)
  • Use an X icon to dismiss, this better indicates that the whole notification can be clicked vs a close button
  • UI affordances for :hover :active :focus states
  • More examples for long titles and descriptions

@Tomastomaslol InVision here(you should be able to use the Inspect feature). I might have to invite you via email if the Inpsect link isn't there.

image

@Tomastomaslol
Copy link
Member Author

Hi @domyen! it looks like you have to invite me for me to be able to access the inspect mode in inVision. Could you send me an invite to tomasnygren.854@gmail.com?

@Tomastomaslol Tomastomaslol force-pushed the 12281_new_version_notification_blocks_content branch from 50d4c36 to a4729bd Compare September 19, 2020 08:38
@Tomastomaslol
Copy link
Member Author

Hi @domyen!

I hope you are well! 🙂

I'm not sure if I'm doing something strange when I export the "book" icon from inVision but I can't figure out how I can get rid of the background in the SVG. If you got a few spare minutes could you please help me export the book icon without the background?

Other than that, I think I got all scenarios covered now:

Screen Shot 2020-09-19 at 5 57 05 pm

Screen Shot 2020-09-19 at 6 00 20 pm

Please let me if something looks wrong or if you would like me to cover any other scenarios!

Thanks!

@domyen
Copy link
Member

domyen commented Sep 30, 2020

Apologies for the delay. Appreciate the work. 🙇

Will review this tomorrow!

@Tomastomaslol
Copy link
Member Author

Hi @domyen!

No worries! Would it be possible to get the book icon as a single string? so I could add it to https://github.com/storybookjs/storybook/blob/next/lib/components/src/icon/icons.tsx#L3 and implement in a similar way to other icons 🙂

@domyen
Copy link
Member

domyen commented Oct 3, 2020

Hey Tomas, apologies again for the delay. Thank you for taking this on, we're super grateful! 🙏

CI
Usually we'd do UI Review in Chromatic but the CI seems to be failing for some reason. Do you mind taking a look at getting CI running again? Maybe there's a fix on next that we can merge into this branch? cc @shilman.

I'll review here instead, but we'll need to get CI green to QA the behavior in the published SB and to merge the code.

Styling

  • Book icon is in the SB icons (go here and cmd + f book)
  • Use the closealt icon for the close button
  • Subheadline should be regular font weight
  • Remove emojis from the stories (we're moving away from this pattern)
  • When there's headline only it should be vertically centered.

Implementation

  • Keep itemContent and itemDismiss inside item. It's fine if item is one big file. Maintaining intertwined functionality across multiple files can be a headache, especially when itemDismiss and itemContent will only be used with the item.
  • The item API should allow for any icon or image to be passed in. Make it a "node" instead so we can add any arbitrary component or DOM to it.

Stories

  • For visual testing purposes, create a story for each discrete state in item

I'm also on Discord if it helps to chat! Find me in the #design channel or @domyen.

@shilman
Copy link
Member

shilman commented Oct 12, 2020

@Tomastomaslol apologies i was messing around with your PR a bit. i think there's some circular dependency that gets created when @storybook/api depends on @storybook/components. i think the solution is to type it as string and then cast it ... WDYT?

@Tomastomaslol
Copy link
Member Author

@shilman thanks for taking your time helping me with this PR. I wasn't sure what the best solution would be but your suggestion makes sense.

There are still some test failures but I'm pretty confident they are not related to the code I wrote and will be fixed with #12746. Please let me know if I have drawn the wrong conclusion and need to spend some more time figuring out why tests are failing.

@domyen I have implemented your feedback. Please let me know if there is anything you would like me to change.

Screen Shot 2020-10-14 at 9 19 10 am
Screen Shot 2020-10-14 at 9 14 43 am

I'm pretty happy with the code and would appreciate a review. 🙂

lib/ui/src/components/notifications/item.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/notifications/item.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/notifications/item.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/notifications/item.stories.js Outdated Show resolved Hide resolved
lib/ui/src/components/notifications/item.tsx Outdated Show resolved Hide resolved
@domyen
Copy link
Member

domyen commented Oct 17, 2020

Thanks for the refinements @Tomastomaslol. 😍

  • I committed some style refinements and reviewed the UI code. See the comments/questions above.
  • Let's get a deeper code review from another maintainer (maybe @ndelangen or @ghengeveld)
  • Once that's done, we're good to go!

lib/api/src/modules/notifications.ts Outdated Show resolved Hide resolved
lib/api/src/modules/notifications.ts Show resolved Hide resolved
lib/ui/src/components/notifications/notifications.tsx Outdated Show resolved Hide resolved
@storybookjs storybookjs deleted a comment from RichardVoyles09 Oct 19, 2020
@Tomastomaslol
Copy link
Member Author

@ndelangen @domyen Thank you for the review. I have now addressed all the points raised in the review. Please let me know if there is something else you would like me to take a look at.

@ndelangen
Copy link
Member

I say it's good to merge!

@shilman shilman modified the milestones: 6.0.x, 6.1 core Oct 21, 2020
@shilman
Copy link
Member

shilman commented Oct 21, 2020

Thanks for your hard work and patience on this @Tomastomaslol 🙏 Merging!

@shilman shilman merged commit 050c0f6 into storybookjs:next Oct 21, 2020
@Tomastomaslol Tomastomaslol deleted the 12281_new_version_notification_blocks_content branch October 22, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants