polaris icon indicating copy to clipboard operation
polaris copied to clipboard

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

Open jorgenunezsiri opened this issue 3 months ago • 5 comments

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

jorgenunezsiri avatar Apr 01 '24 21:04 jorgenunezsiri

@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.

chloerice avatar Apr 02 '24 11:04 chloerice

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.

heyjoethomas avatar Apr 02 '24 14:04 heyjoethomas

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"?

chloerice avatar Apr 02 '24 17:04 chloerice

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:

https://github.com/Shopify/polaris/assets/16692690/673ca39c-acca-49da-b280-1c9c96143116

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).

jorgenunezsiri avatar Apr 02 '24 21:04 jorgenunezsiri

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.