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 vote button variant #1685

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Conversation

dancormier
Copy link
Contributor

@dancormier dancormier commented Mar 21, 2024

This PR adds a vote variant of the button component to more tightly control and customize vote button styling via Stacks.

Deploy preview: https://deploy-preview-1685--stacks.netlify.app/product/components/buttons/#vote

Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 0d04210
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/65fc8e7b8f7f0d0008580956
😎 Deploy Preview https://deploy-preview-1685--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.

@giamir
Copy link
Contributor

giamir commented Mar 22, 2024

@dancormier @CGuindon What do you think about "dog feed" this new button styles in our docs and replace the existing feedback buttons (see screenshot)?
Screenshot 2024-03-22 at 13 11 58

@giamir
Copy link
Contributor

giamir commented Mar 22, 2024

@CGuindon @dancormier I am not sure how to feel about the focus state changing the internal background color of the button with different colors depending if it is selected or not.
Screenshot 2024-03-22 at 13 38 05
Screenshot 2024-03-22 at 13 38 23
Screenshot 2024-03-22 at 13 38 47
Screenshot 2024-03-22 at 13 38 59

Would not be simpler and more intuitive to treat this special vote button like we do toggles or tags? I mean just surrounding the element with an outline and leave the background colors unchanged.
Screenshot 2024-03-22 at 13 39 52
Screenshot 2024-03-22 at 13 39 44

I am thinking that if I struggle to understand which color should the vote button be when focused and I work in Stacks I can only imagine how difficult it would be for others to wrap their heads around it.

@giamir
Copy link
Contributor

giamir commented Mar 22, 2024

@dancormier Could we remove the arrow icon button example in the icons section of the page? I think it could be confusing for people since now we have a dedicate vote variant.

Screenshot 2024-03-22 at 13 45 13


<div class="stacks-preview">
{% highlight html %}
<button class="s-btn s-btn__vote" type="button">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add the appropriate aria attributes to this example since it is used in most places as a toggle: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed

{% highlight html %}
<button class="s-btn s-btn__vote" type="button">
@Svg.ArrowUp
<span class="v-visible-sr">Upvote</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be more elegant and shorter to use an aria-label in the button instead?

@giamir
Copy link
Contributor

giamir commented Mar 22, 2024

@dancormier @CGuindon Is vote the right name for the variant? I think it would be best to describe the variant from a stylistic perspective instead of semantically. I am saying this because we use the same button for bookmarking already and I can imagine how it could be used for other purposes too in the future.
Screenshot 2024-03-22 at 13 59 33

round variant perhaps? I am sure we can come up with a good (domain-agnostic) name.

@giamir
Copy link
Contributor

giamir commented Mar 22, 2024

@dancormier @CGuindon I think this is a good starting point to standardize the vote (or round or whatever 🙂) buttons in Stacks but I think it could benefit from us reflecting some more on our choices before shipping it. We certainly should not treat this work as "side" work especially because I imagine it will be quite painful to integrate and test this changes across different places in Core. I would recommend to plan this extraction work for one of our next sprints and address anything urgent in Core with a temporary fix. We can talk more in our planning today and it's totally ok if you disagree. 🙂

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.

2 participants