Refactor Flash alert #1979

vdepizzol · 2022-03-03T23:49:48Z

What are you trying to accomplish?

This PR adds a new Flash component, meant to eventually replace our existing flash alert. This update aims to fix long standing issues found in it, mainly regarding responsive support.

✨ Link to Storybook ✨

Main updates:

Improved alignment and spacing

Responsive support

A lot of the spacing values used in this refactor are hardcoded, and will benefit from the Primer Primitives spacing update once that's available. (link only available for Hubbers).

What approach did you choose and why?

I've kept the current flash alert in place, and created a new flash-refactor.scss file, in a similar fashion to how @langermank has considered the refactor for the Button component (#1874).

After all instances of flash are replaced with this new component (specially after updating its ViewComponent, we can clean up these files.

Are additional changes needed?

As described in https://github.com/github/primer/issues/62 (internal to hubbers), some alerts may have multiple buttons. This refactor hasn't addressed for these use cases specifically.

No, this PR should be ok to ship as is. 🚢

Closes https://github.com/github/primer/issues/154.

changeset-bot · 2022-03-03T23:49:50Z

🦋 Changeset detected

Latest commit: 2b59674

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

This PR includes changesets to release 1 package

Name	Type
@primer/css	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

langermank

Looks great! A few questions but not blocking 🙂

langermank · 2022-03-04T01:29:58Z

docs/src/stories/components/Alerts/Flash.stories.jsx

+          </div>
+        )}
+
+        <div className={clsx('Flash-message')} dangerouslySetInnerHTML={{__html: message}}></div>


What do you think about creating a slot for title and description? The <strong> and <br> feel like something we should handle at the component level.

+1 to what @langermank is suggesting here. I think that this could be similar to the Blankslate API where both the title and description are styled based on component classes.

langermank · 2022-03-04T01:34:18Z

src/alerts/flash-refactor.scss

+    grid-area: actions;
+
+    &:last-child {
+      align-self: end;


This might be out of scope (I'm not sure if you're working from existing designs) but the trailing action alignment stands out to me. I feel like I want it to align with the text. What do you think?

Or keep at start so it aligns with the icon regardless how much content there is:

My line of thought was that, for banners with multiple lines and no dismiss button, the call-to-action should be aligned to the end of the message, like: "read the message before looking at the button".

If the banner does have a dismiss button, though, I'm still keeping it aligned to the top. For some reason I was imagining that banners that have a dismiss button have call-to-actions that are not as important, since they can be dismissed 🤔 .

Seeing the banners above with the different alignments can definitely seen as jarring, but these usually don't appear next to each other anyways.

@langermank @simurai Do you think that's too weird? 😆

the call-to-action should be aligned to the end of the message, like: "read the message before looking at the button".

I see. Yeah, I think that makes sense. Let's give it a try.

I think it's okay to allow for two different action button positions 👍

langermank · 2022-03-04T01:34:37Z

src/alerts/flash-refactor.scss

+  border: $border-width $border-style var(--color-accent-muted);
+  border-radius: $border-radius;
+  grid-auto-flow: column;
+  grid-template-areas: 'visual message actions close';


Beautiful! ☺️

langermank · 2022-03-04T01:35:01Z

src/alerts/flash-refactor.scss

+    align-self: start;
+
+    > .octicon {
+      margin-block: calc((#{$spacer-1}) / 2);


oOOooo fancy

simurai · 2022-03-04T08:18:59Z

src/alerts/flash-refactor.scss

+    grid-area: actions;
+
+    &:last-child {
+      align-self: end;


Or keep at start so it aligns with the icon regardless how much content there is:

simurai · 2022-03-04T08:27:59Z

src/alerts/flash-refactor.scss

+
+  // Replace with Primitives' Narrow viewport range
+  @media (max-width: #{map-get($breakpoints, 'md') - 0.02px}) {
+    &.Flash--full-whenNarrow {


full-whenNarrow 👍 This looks useful.

@simurai I'm already using it for #1898 (Example in Storybook) 🎉 🎉

Yesss... this, exactly!

simurai · 2022-03-04T08:41:32Z

src/alerts/flash-refactor.scss

+  .Flash-visual {
+    display: grid;
+    // stylelint-disable-next-line primer/spacing
+    padding: calc((#{$spacer-5} - 20px) / 2) $spacer-2;


Is there a specific reason to use calc() here and 2 more times below? I think the browser will see it as:

padding: calc((32px - 20px) / 2) 8px;

but because they are static px values it won't change. So we could also just use the calculated px value and probably improve performance a tiny bit:

padding: 6px $spacer-2;

If it was to get around the stylelint-disable, @jonrohan patched that recently. 😉

@simurai I'm getting prepared for the use of functional spacing design tokens (link only available for hubbers). $spacer-5 should be --gh-control-medium-size, and 20px --gh-text-body-medium-lineBoxHeight. This calculation adds any extra padding needed so the 16px octicon gets vertically centered with the first line of text, even in case there are multiple lines.

I'll add this as a comment in the file. 🙇

I'm getting prepared for the use of functional spacing design tokens

ahhh.. ok ok, in that case let's keep as is for easier updating later. 👍

vdepizzol · 2022-03-04T19:05:15Z

docs/src/stories/components/Alerts/Flash.stories.jsx

+        type: 'boolean',
+      },
+    },
+    hasCloseButton: {


After chatting with @ashygee, I'll rename this to "Dismiss button".

vdepizzol · 2022-03-04T19:08:07Z

src/alerts/flash-refactor.scss

+
+// Flash alert
+
+.Flash {


After chatting with @ashygee, we agreed to rename this component to "Alert".

Additionally, @alliethu gave some great insights into how to handle accessibility for it:

A couple alert notes:

Use role="alert" by default

If more than one alert can display at once, use role="status" so the screen reader doesn’t interrupt itself with the latest message.

Provide a cue for the alert type (info, warning, error, success)

Visually with an icon

Audibly using visually hidden text that precedes the alert

If no dismiss/close button, the alert must be present for a minimum of 5 seconds before dismissing.

Connect title and description using aria-describedby

@vdepizzol based on the conversation with @langermank I think that "Banner" is a better name for this component. If we do decide to keep "Alert" though we may want to clarify the difference between this and a Toast notification/alert. I also think we may want to audit the usage of the Toast since this alert styling is much more common across the patterns seen within the GitHub experience.

@ashygee just renamed the component to Banner.

I haven't updated the stories with WAI-ARIA considerations because I think these are still in flux. Per instance, there are many existing scenarios¹ where role="alert" shouldn't be useful. I think we can polish the accessibility definitions directly in the PVC updates.

Footnotes

Link only available to hubbers. ↩

vdepizzol · 2022-03-04T19:09:42Z

I'm moving this back to a draft to update the proposal with full support for accessibility and better naming. Thank you @alliethu and @ashygee 🎉 !

ashygee · 2022-03-09T00:45:41Z

@vdepizzol after our conversation we had discussed that the full version of the Banner (Flash) component would be what we currently style as .flash-banner, see screenshot below. I did not see this reflected in Storybook as it still has the border at the top and bottom of the component. Are we still considering making this change?

vdepizzol · 2022-03-14T23:45:09Z

@ashygee:

after our conversation we had discussed that the full version of the Banner (Flash) component would be what we currently style as .flash-banner, see screenshot below. I did not see this reflected in Storybook as it still has the border at the top and bottom of the component. Are we still considering making this change?

So, I'm no longer including a -banner variant in the component since we'll no longer offer support for sticky banners. From the Primer CSS docs:

A flash message that is fixed positioned at the top of the page. Use for more global events where the content of the page is unknown.

Eventually a system tray should better support system banners. See #1898.

In the meantime, a -full variant should still work inside bordered Box elements:

More about Boxes with alerts

✌️ ✌️ ✌️

simurai

Rename Flash to Banner

Was also thinking if it should just be called Alert? But maybe it's still good to keep it as a "category" in case we have other kinds of alerts in the future.

langermank · 2022-03-15T21:20:15Z

docs/src/stories/components/Alerts/Banner.stories.jsx

+          <p className={clsx('Banner-title')} dangerouslySetInnerHTML={{__html: title}}></p>
+          {description && (
+            <>
+              <p className={clsx('Banner-description')} dangerouslySetInnerHTML={{__html: description}}></p>


I see Banner-description here but not in the CSS, did you mean to add some styles for this guy?

…lash-refactor

langermank · 2022-03-15T21:24:11Z

src/alerts/banner.scss

+      margin-bottom: 0;
+    }
+
+    .Banner-title:not(:only-child) {


This is clever ✨ I guess just delete that description class from the stories and use <p>stuff</p>?

github-actions · 2022-06-19T20:05:23Z

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

siddharthkp · 2022-08-10T15:49:00Z

👋 @vdepizzol How do you feel about merging this component and start testing it out in production?

simurai · 2022-08-15T05:47:02Z

How do you feel about merging this component and start testing it out in production?

Is there already a need for this in production?

I think the main reason this hasn't shipped is that there is no plan for the PVC implementation yet. And we want to avoid using it with custom markup/HTML. So it would just be un-used CSS for now.

siddharthkp · 2022-08-15T10:38:37Z

Is there already a need for this in production?

No rush, curious because some of the problems identified in this thread were reported again when I was first responder

ashygee · 2022-08-17T19:53:41Z

Is there already a need for this in production?

No rush, curious because some of the problems identified in this thread were reported again when I was first responder

FWIW, it's been shipped in Figma.

camertron · 2022-10-13T23:09:24Z

Hey everyone! The PVC team has picked this up, see: primer/view_components#1494

jonrohan

blocking for primer/view_components#1494

github-actions · 2022-12-13T00:21:19Z

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

simurai · 2022-12-13T02:06:00Z

Closing this again since the .Banner styles now live in PVC.

Add refactored Flash component

72651dc

vdepizzol requested a review from a team as a code owner March 3, 2022 23:49

vdepizzol requested a review from langermank March 3, 2022 23:49

vdepizzol mentioned this pull request Mar 4, 2022

Flash alert issues #1140

Closed

Please stylelint errors

5881331

github-actions bot deployed to Preview March 4, 2022 00:55 View deployment

github-actions bot deployed to Storybook Preview March 4, 2022 00:55 View deployment

Please stylelint errors (2)

35a4d12

github-actions bot deployed to Preview March 4, 2022 00:59 View deployment

github-actions bot deployed to Storybook Preview March 4, 2022 00:59 View deployment

vdepizzol requested a review from simurai March 4, 2022 01:03

langermank approved these changes Mar 4, 2022

View reviewed changes

simurai reviewed Mar 4, 2022

View reviewed changes

vdepizzol commented Mar 4, 2022

View reviewed changes

vdepizzol marked this pull request as draft March 4, 2022 19:08

Add design token comments

cebfff1

github-actions bot deployed to Preview March 5, 2022 02:15 View deployment

github-actions bot deployed to Storybook Preview March 5, 2022 02:15 View deployment

vdepizzol added 3 commits March 14, 2022 15:10

s/Flash/Banner, add title+description slots

20bc751

Rename Flash to Banner

64bb053

Clean up Banner component

bdcc9cb

github-actions bot deployed to Preview March 14, 2022 22:24 View deployment

github-actions bot deployed to Storybook Preview March 14, 2022 22:24 View deployment

Stylelint auto-fixes

ea1aab5

vdepizzol marked this pull request as ready for review March 14, 2022 23:45

Merge branch 'main' into flash-refactor

d2ad047

github-actions bot deployed to Preview March 15, 2022 01:33 View deployment

github-actions bot deployed to Storybook Preview March 15, 2022 01:33 View deployment

simurai approved these changes Mar 15, 2022

View reviewed changes

langermank reviewed Mar 15, 2022

View reviewed changes

vdepizzol added 2 commits March 15, 2022 14:23

Update message with title and description

0f03695

Merge branch 'flash-refactor' of https://github.com/primer/css into f…

5a3de92

…lash-refactor

langermank reviewed Mar 15, 2022

View reviewed changes

github-actions bot deployed to Preview March 15, 2022 21:24 View deployment

github-actions bot deployed to Storybook Preview March 15, 2022 21:24 View deployment

Create pretty-dolls-explain.md

753e7db

jonrohan added the minor release label Apr 20, 2022

jonrohan temporarily deployed to github-pages April 20, 2022 16:32 Inactive

Merge branch 'main' into flash-refactor

2b59674

jonrohan deployed to github-pages April 20, 2022 19:40 View deployment

github-actions bot added the Stale Automatically marked as stale. label Jun 19, 2022

simurai removed the Stale Automatically marked as stale. label Jun 20, 2022

camertron mentioned this pull request Oct 13, 2022

Updates to the flash component to use new banner styles primer/view_components#1494

Closed

3 tasks

jonrohan requested changes Oct 14, 2022

View reviewed changes

github-actions bot added the Stale Automatically marked as stale. label Dec 13, 2022

simurai closed this Dec 13, 2022

simurai deleted the flash-refactor branch December 13, 2022 02:06

Refactor Flash alert #1979

Refactor Flash alert #1979

Conversation

vdepizzol commented Mar 3, 2022 • edited

What are you trying to accomplish?

What approach did you choose and why?

Are additional changes needed?

changeset-bot bot commented Mar 3, 2022 • edited

🦋 Changeset detected

langermank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdepizzol Mar 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

vdepizzol commented Mar 4, 2022

ashygee commented Mar 9, 2022

vdepizzol commented Mar 14, 2022

simurai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 19, 2022

siddharthkp commented Aug 10, 2022

simurai commented Aug 15, 2022

siddharthkp commented Aug 15, 2022

ashygee commented Aug 17, 2022

camertron commented Oct 13, 2022

jonrohan left a comment • edited

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2022

simurai commented Dec 13, 2022

vdepizzol commented Mar 3, 2022 •

edited

changeset-bot bot commented Mar 3, 2022 •

edited

vdepizzol Mar 4, 2022 •

edited

jonrohan left a comment •

edited