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

feat(button): add "featured" variant #1857

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

dancormier
Copy link
Contributor

@dancormier dancormier commented Nov 14, 2024

STACKS-680

TODO

Note

I've opened this PR up for review but there's a couple of outstanding items that need to be resolved:

  • Verify name "featured"
  • Write description in docs

This PR adds a "featured" (purple) variant to the button component.

Screenshots

Light

image

Dark

image

Light high contrast

image

Dark high contrast

image

Testing

  1. Go to /product/components/buttons/#featured
  2. Verify the newly added featured buttons look and act as expected

Notable changes

High contrast filled background color

To meet accessibility requirements, the featured filled variant background color has been set to --purple-500 (normally --purple-400).

Borders on filled, selected buttons

This PR includes a minor fix to button border colors when hovered and filled and/or selected. Previously, the value of --bu-bc (currentColor) was applied and now the border color applied is equal to --bu-bg, ensuring the border and button background are unified.

Button group visual regression test images

There are six visual regression tests (all on s-btn-group-highcontrast-* in Chromium runs) that have been updated. They are visually identical to me but there could be extremely slight differences due to 838e25a.

Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 6c2242d
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/67479fb46a67a00008753711
😎 Deploy Preview https://deploy-preview-1857--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

When filled/selected buttons were hovered, the currentColor would be used for the border color. This commit ensures the correct border color is used on filled/selected buttons
@dancormier dancormier changed the title WIP: feat(button): add "featured" variant feat(button): add "featured" variant Nov 15, 2024
@dancormier dancormier requested a review from a team November 15, 2024 18:27

{% header "h3", "Featured" %}
<!-- TODO add featured button description -->
<p class="stacks-copy">Featured buttons are…</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CGuindon Here's where the description for the "featured" buttons would go. Feel free to make a commit or pass a description along to me and I'll plug it in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't push to Stacks so here you go — let me know what you think. I looked at the other buttons and the color purple description in the color fundamentals:

Featured buttons are a secondary button style, used to visually draw attention to something new or temporary, usually as part of onboarding or to announce a new feature. These should be used sparingly, and permanent placements should be avoided

@@ -35,6 +35,26 @@
}
]
}
]
,
"featured": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with featured for this variant name.

I figured it's best to avoid coupling naming with visual that could change (purple) and new felt too restrictive since I could see this button being used just to grab some attention generally. featured felt like the best fit but I'm glad to change it if you feel differently @CGuindon.

@dancormier dancormier marked this pull request as ready for review November 19, 2024 19:11
Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Overall looks good, there is only a minor docs correction to do I left in comment.

Nice to see the PPCP abstraction making our lives easier here.
Well done @dancormier ❤️

docs/_data/buttons.json Show resolved Hide resolved
docs/_data/buttons.json Show resolved Hide resolved
docs/_data/buttons.json Show resolved Hide resolved
Addresses #1857 (comment) and similar
Copy link
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

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

Ok so the only thing I don't love is that in HC mode, the filled default state is the same color as the filled selected state (no difference).

Because we had to bump the background color from 400 to 500 for HC filled, and because 500 and 600 are the same in HC, we end up with no difference there.

For Base and Danger we have a modified red-400 and blue-400 just for HC mode. Those colors were modified from Light/Dark so that they meet the 7:1 ratio (they're pretty close but not quite otherwise). And so for those variants we're able to keep the 400 stop as the background color for filled default and therefore have a color difference with the selected state.

So....

I'm proposing we modify HC modes purple 400 color to meet a 7:1 ratio with white.
Change the selected state back to using purple-400 but with this new color (in dark mode the purple 400 already passes 7:1 ratio.

  • HC Light purple-400 would become #3B41BA (Figma where I played with this)
  • HC Dark purple-400 would become NO CHANGE NEEDED? since it already passes the 7:1 ratio?

Screenshot 2024-11-27 at 3 08 26 PM
Screenshot 2024-11-27 at 3 08 47 PM

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.

3 participants