Skip to content

Conversation

AriannaChau
Copy link
Contributor

@AriannaChau AriannaChau commented Oct 4, 2021

Overview

This PR adds two new modifiers for the alert pattern: Adding an icon and adding a floating style.
There's also an alert added to a prototype to get a feel for the intended use of the offline alert.

A couple of other issues can come from this:

  • Add more icon background colors
  • Adding white background as its own modifier?
  • Add positioning for specific offline alert
  • Create a new offline/wifi icon

Screenshots

Icon modifier:

Screen Shot 2021-10-04 at 3 00 51 PM

Floating modifier:

Screen Shot 2021-10-04 at 3 01 01 PM

Alert in practice:

Screen Shot 2021-10-04 at 3 00 11 PM

Testing

  1. Deploy link (icon)
  2. Deploy link (floating)

@changeset-bot
Copy link

changeset-bot bot commented Oct 4, 2021

🦋 Changeset detected

Latest commit: 4152711

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

This PR includes changesets to release 1 package
Name Type
@cloudfour/patterns 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

@netlify
Copy link

netlify bot commented Oct 4, 2021

✔️ Deploy Preview for cloudfour-patterns ready!

🔨 Explore the source changes: 4152711

🔍 Inspect the deploy log: https://app.netlify.com/sites/cloudfour-patterns/deploys/6195919f3454900007c37535

😎 Browse the preview: https://deploy-preview-1557--cloudfour-patterns.netlify.app

@AriannaChau AriannaChau marked this pull request as ready for review October 4, 2021 22:09
Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

This is looking great, and I think the list of future issues is spot on.

I left some comments inline. I also notice that when I apply the dark theme to the story, the text disappears.

You may want to consider theming the alert for light and dark containing themes, but then supporting adding a theme class (such as t-light) directly so you'll get the text, link and background colors where desired.

Screen Shot 2021-10-04 at 3 32 26 PM

@dromo77
Copy link
Contributor

dromo77 commented Nov 15, 2021

@tylersticka I'm picking up where Arianna left off with this PR. It looks like she addressed almost all the feedback. I resolved a couple merge conflicts and addressed a small piece of feedback that looked like it hadn't been updated yet. Would you mind taking a look again?

@dromo77 dromo77 requested a review from tylersticka November 15, 2021 23:16
Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

Something looks amiss when the alert is full-width at very wide sizes: https://deploy-preview-1557--cloudfour-patterns.netlify.app/?path=/story/components-alert--icon

Screen Shot 2021-11-15 at 3 42 35 PM

@dromo77
Copy link
Contributor

dromo77 commented Nov 16, 2021

Something looks amiss when the alert is full-width at very wide sizes

@tylersticka c-alert__inner has a max-width equal to our size.$max-width-spread token. When the alert only has text, the alert content lines up with the page content. But it's funky when there's an icon. I'm not sure what the best solution. We could remove the max-width with a modifier when there is an icon present. The downside of this is that some alert messages will be constrained and others won't. We could remove the max-width altogether, but that would allow alert content to span the full width of its container. We might also consider changing the style of the icon a bit on very large screens so that it works with the max-width. This is a quick example, but maybe something like this?

Screen Shot 2021-11-16 at 11 59 18 AM

@tylersticka
Copy link
Member

This is a quick example, but maybe something like this?

@dromo77 I like that idea, it feels the most flexible and responsive. It might look even more purposeful and "on-brand" if the container became a circle past that max-width size?

@dromo77 dromo77 requested a review from tylersticka November 16, 2021 21:38
Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

The responsiveness feels great now! Some more feedback left inline, reach out if you have questions or aren't sure where to go next.

l0.5-0.5c3.9-3.9,10.3-3.9,14.2,0l0.5,0.5c0.1,0.1,0.3,0.2,0.4,0.2s0.3-0.1,0.4-0.2l0.5-0.5C21.3,10.1,21.3,9.7,21,9.5z"/>
<path d="M12,23c6.1,0,11-4.9,11-11S18.1,1,12,1S1,5.9,1,12S5.9,23,12,23z M19.8,5.1l-16,13.7" fill="none" stroke-width="2"/>
</svg>

Copy link
Member

Choose a reason for hiding this comment

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

This icon feels like it could be more consistent with other elements of our patterns.

I think this is especially clear if you compare it to the way our bell icon looks inside of a slashed button:

Screen Shot 2021-11-16 at 2 10 06 PM

Screen Shot 2021-11-16 at 2 10 18 PM

When searching the Noun Project for "offline," I notice that a common visual is a cloud with a slash through it. Have we missed an opportunity to use a cloud where it makes symbolic sense? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of using a cloud. What do you think of this @tylersticka?

Screen Shot 2021-11-17 at 8 47 20 AM

Copy link
Member

Choose a reason for hiding this comment

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

I like that better. Is it possible to make the cloud larger in relation to the slash?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's subtle, but here's an update version with the cloud a little larger in proportion to the slash.

Screen Shot 2021-11-17 at 9 42 12 AM

Copy link
Member

Choose a reason for hiding this comment

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

I like that better!

Can the icon itself be larger within its container? I think I'm reacting to its size relative to the capital "Y" in "You"

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it is a little bigger:

Screen Shot 2021-11-17 at 10 25 03 AM

Copy link
Member

Choose a reason for hiding this comment

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

I like that! How do you feel about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it too!

@dromo77 dromo77 requested a review from tylersticka November 17, 2021 17:52
Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

Something I just noticed: In general, we want to name our icons based on what they are, not what they're for. So instead of offline, we might name it cloud-slash.

That might seem silly, but in other projects we've run into issues where we name an icon after one feature, it ends up being used for something else, and then later we stop using it for the original feature, so it's sort of in limbo. Better to just name it what it is.

Otherwise, this is great!

@dromo77 dromo77 requested a review from tylersticka November 17, 2021 23:44
@dromo77 dromo77 merged commit ba57050 into v-next Nov 18, 2021
@dromo77 dromo77 deleted the patterns/alert-variation branch November 18, 2021 00:01
@github-actions github-actions bot mentioned this pull request Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offline variation of alert pattern
3 participants