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

[Button] Add support for two right direction icons in disclosure prop. #11812

Closed

Conversation

jorgenunezsiri
Copy link
Contributor

@jorgenunezsiri jorgenunezsiri commented Apr 1, 2024

WHY are these changes introduced?

Part of https://github.com/Shopify/web/issues/110696

Reasoning: As part of the signup questionnaire in Admin, we have a temporary DisclosureIconButton component that was created as a forked local version of the Button component, to add support for two right direction icons in the disclosure prop:

23-12-2a83e-16gvr

Having these two icons present in the disclosure prop of the Button component directly would allow us to simplify this implementation, by removing the previous temporary DisclosureIconButton component and using Button component for these Admin use cases.

WHAT is this pull request doing?

This PR adds supports for two right direction icons in Button component disclosure prop: ChevronRightIcon and ArrowRightIcon, which are used to indicate in-context navigation or navigation to another context, respectively.

Results in Storybook

  • Chevron right disclosure:
chevron-right-disclosure
  • Arrow right disclosure:
arrow-right-disclosure

How to 🎩

  • All Button component tests should pass.
  • Storybook displays two new options for disclosure prop in Button component (see results above).

🎩 checklist

The two new disclosure icons are `ChevronRightIcon` and `ArrowRightIcon`,
which are used to indicate in-context navigation or navigation to
another context, respectively.
@jorgenunezsiri jorgenunezsiri self-assigned this Apr 1, 2024
@jorgenunezsiri jorgenunezsiri requested review from chloerice and a team April 1, 2024 21:02
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

@jorgenunezsiri the disclosure naming is for creating a popover after a button press. Why can't you do this with an icon in a button?

@chloerice
Copy link
Member

chloerice commented Apr 2, 2024

@jorgenunezsiri the disclosure naming is for creating a popover after a button press. Why can't you do this with an icon in a button?

Button.icon puts the icon on the left side of the Button. This is techinically still a disclosure pattern since it opens the next page of a paged Modal, PositionedOverlay is also about to support horizontal position so that Popover and HoverCard can open left/right. Using UnstyledButton instead would mean copying over hundreds of lines of CSS from Button.css into Web when this is a simple thing that makes sense to support in the system.

@heyjoethomas
Copy link
Contributor

heyjoethomas commented Apr 2, 2024

I think the ux here could converge into a single option. I don't see a lot of value in a chevron and arrow right option.

@chloerice
Copy link
Member

chloerice commented Apr 2, 2024

I think the ux here could converge into a single option. I don't see a lot of value in a chevron and arrow right option.

+1, the arrow doesn't add clarity in my opinion. Where does "I don't want help setting up" navigate the merchant? Was there any exploration into a simplified dismissal action in context, like "Skip" or "Dismiss"?

@jorgenunezsiri
Copy link
Contributor Author

Where does "I don't want help setting up" navigate the merchant? Was there any exploration into a simplified dismissal action in context, like "Skip" or "Dismiss"?

@chloerice Thank you for your feedback! There are actually three use cases to consider where we are using the temporary DisclosureIconButton component nowadays:

  • (1) Next > button: The chevron right icon here indicates in-context navigation within the signup questionnaire - similar to how the chevron left icon is presented in the < Back button.
  • (2) & (3) I don't want help setting up -> button / Get started -> button: These two buttons use an arrow right to indicate that we are moving away from the signup questionnaire context to the Admin home page context, where you get your home cards, home guides, and home feed.

Please see the video below where I highlight the use cases and also the transitions between the questionnaire cards and from the questionnaire context to the Admin home page context:

signup-questionnaire-in-admin.mov

Button.icon puts the icon on the left side of the Button. This is techinically still a disclosure pattern since it opens the next page of a paged Modal, PositionedOverlay is also about to support horizontal position so that Popover and HoverCard can open left/right. Using UnstyledButton instead would mean copying over hundreds of lines of CSS from Button.css into Web when this is a simple thing that makes sense to support in the system.

@chloerice Yes, this is exactly right. I also agree that this is technically still a disclosure pattern. For the temporary DisclosureIconButton component we were able to minimize the styling copy & paste since we are only using the tertiary variant for these buttons. But, in the end, it would make sense to support this in the Polaris button for us to decrease maintenance burden in the future. This is because right now the < Back button is a Polaris button, but the other questionnaire navigation buttons are not, so if something ever changes in the Polaris side Button implementation in the future, it would need to be manually ported to the other buttons (three cases highlighted above).

@aaronccasanova aaronccasanova force-pushed the jorgenunezsiri/improve-button-disclosure-api branch from 4c3d641 to c227341 Compare April 19, 2024 21:52
@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024
@translation-platform
Copy link
Contributor

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

  • The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-needed Added by a bot. Contributor needs to sign the CLA Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants