-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[UserMenu] Update Topbar User Menu and Action List to reflect new designs #8856
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
Conversation
|
/snapit |
|
🫰✨ Thanks @zakwarsame! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230404213853yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230404213853yarn add @shopify/polaris@0.0.0-snapshot-release-20230404213853yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230404213853 |
size-limit report 📦
|
kyledurand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking ok to me, just a few comments. I'd like to hear what others think as well before shipping
polaris-react/src/components/ActionList/components/Section/Section.tsx
Outdated
Show resolved
Hide resolved
667299a to
61da173
Compare
| let suffixMarkup: React.ReactNode | null = null; | ||
|
|
||
| if (active) { | ||
| suffixMarkup = ( | ||
| <Box> | ||
| <span className={styles.Suffix}> | ||
| <Icon source={TickSmallMinor} /> | ||
| </span> | ||
| </Box> | ||
| ); | ||
| } else if (suffix) { | ||
| suffixMarkup = suffix && ( | ||
| <Box> | ||
| <span className={styles.Suffix}>{suffix}</span> | ||
| </Box> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tentatively made the suffix to always be TickSmallMinor when active is true following @chloerice's suggestion.
Will update if the consensus changes.
1c53e45 to
2dfb8b9
Compare
2dfb8b9 to
f25f40d
Compare
dd7b57b to
6e77715
Compare
|
/snapit |
|
🫰✨ Thanks @zakwarsame! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230411134611yarn add @shopify/polaris-codemods@0.0.0-snapshot-release-20230411134611yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230411134611yarn add @shopify/polaris@0.0.0-snapshot-release-20230411134611yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230411134611yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230411134611 |
|
/snapit |
|
🫰✨ Thanks @zakwarsame! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230412181419yarn add @shopify/polaris-codemods@0.0.0-snapshot-release-20230412181419yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230412181419yarn add @shopify/polaris@0.0.0-snapshot-release-20230412181419yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230412181419yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230412181419 |
…tionlist title; allow helptext as prop to sections fix: commits that are messed up ammend: use source path ammend: call hook outside of callback review: use unique id on iteration ammend: use source path ammend: call hook outside of callback review: use unique id on iteration Update .changeset/famous-hairs-jam.md fix: use correct import on usermenu; use correct key changeset chore: update top bar menu stories to reflect new menu chore: use actionlistprops as type of actions review: remove helptext from sections review: update string type fix: move import line fixup fix: make tick minor the default when active is true; use typeof for key fix: use unique id combined with index fix: remove string from id generation Co-Authored-By: Kyle Durand <kyledurand@users.noreply.github.com>
5d4d68f to
21558bf
Compare
polaris-react/src/components/TopBar/components/UserMenu/UserMenu.tsx
Outdated
Show resolved
Hide resolved
| '@shopify/polaris': minor | ||
| --- | ||
|
|
||
| Updated top bar menu active state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Updated top bar menu active state | |
| - Updated `TopBar.UserMenu` active state styles | |
| - Added the `truncate` prop to `ActionList.Item` | |
| - Added support for setting an array of type ActionListSection[] on the `Topbar.UserMenu` `actions` prop | |
| /** Add an ellipsis suffix to action content */ | ||
| ellipsis?: boolean; | ||
| /** Truncate action content */ | ||
| truncate?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyle Should ellipsis be depreecated in favor of truncate? It seems like the ellipsis prop is pointless as is and should have been doing actual truncation all this time since the content prop is string only (can't be passed in as truncated) 🤔
| <div className={styles.Trimmed}> | ||
| <Truncate>{trimmedText}</Truncate> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <div className={styles.Trimmed}> | |
| <Truncate>{trimmedText}</Truncate> | |
| </div> | |
| <Text truncate as="p" variant="bodySm">{trimmedText}</Text> |
The undocumented Truncate component is going to be removed in favor of Text.truncate.
| truncate | ||
| />, | ||
| ); | ||
| expect(item).toContainReactComponent(Truncate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| expect(item).toContainReactComponent(Truncate); | |
| expect(item).toContainReactComponent(Text, {truncate: true}); |
| { | ||
| title: 'Dharma Johnson', | ||
| items: [{content: 'Manage account'}, {content: 'Log out'}], | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all user menus going to have the store switcher eventually instead of the Stores action list item that links out to an identity page? If not, should this use case be documented as its own story since it's unique to merchants who have the store switcher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Our current project aims to roll out the new store switcher to all our merchants except Plus, who have a separate nav.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay! In that case, should we:
- Make this a layout change internal to the component instead of adding a
customActivatorprop, since the pattern is being updated across the board? - Add an optional
logoprop that renders aThumbnailto the right of thename(instead of anavataron the left) when provided, and makeinitialsoptional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference: moved
Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
|
I ran into a couple of more things that need scrutiny with regards to updating the
Makes me wonder if we should have a separate |
| } | ||
|
|
||
| const contentText = ellipsis && content ? `${content}…` : content; | ||
| let contentText = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const contentText = ellipsis && content ? `${content}…` : (truncate ? truncateText(content) : content);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Nesting a ternary operator is shorter, but the if else style is more readable to other devs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should consider using const most if not all places, and your condition is duplicated!
const contextText = content
if (ellipsis && content) {
contentText = `${content}…`;
} else if (content && truncate) {
contentText = truncateText(content);
} remove the else, and assign contextText = content as default.
but i still prefer one line, since its very short condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer clarity over brevity, especially in this case because theres multiple props that influence what's rendered and one needs to take precedence.
| let contentText = null; | |
| let contentText = content | |
| if (ellipsis) { | |
| contentText = `${content}…`; | |
| } else if (truncate) { | |
| contentText = truncateText(content); | |
| } |
I do think that ellipsis should be deprecated in favor of the truncate prop you're adding @zakwarsame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on it here 😄
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Resolves https://github.com/Shopify/core-workflows/issues/881 Follow up to: #8856 <!-- Context about the problem that’s being addressed. --> Part of [Commerce Components by Shopify](https://www.shopify.com/commerce-components); which requires some customization to the user menu ### WHAT is this pull request doing? `Topbar.UserMenu`: Allow a custom `activatorContent` prop to be passed enabling us to customize the order of the avatar and heading. <details> <summary> Expand to see before/after </summary> | Before | After | | ------------ |:-------------:| |  |  | </details> `ActionList`: `title` is updated to accept a `React.Node` instead of string. <details> <summary>Expand for screenshot </summary>  </details> <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 <!-- Give as much information as needed to experiment with the component in the playground. --> - [Spin URL](https://admin.web.acc-menu.zakaria-warsame.us.spin.dev/store/shop1) - Notice how we're able to pass a custom activator with a squared avatar, and a different text style - The `title` "Test User" is also customized to use a different style. Passing just a string would still have the original style. ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide --------- Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
…ify#8953) <!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Resolves https://github.com/Shopify/core-workflows/issues/881 Follow up to: Shopify#8856 <!-- Context about the problem that’s being addressed. --> Part of [Commerce Components by Shopify](https://www.shopify.com/commerce-components); which requires some customization to the user menu ### WHAT is this pull request doing? `Topbar.UserMenu`: Allow a custom `activatorContent` prop to be passed enabling us to customize the order of the avatar and heading. <details> <summary> Expand to see before/after </summary> | Before | After | | ------------ |:-------------:| |  |  | </details> `ActionList`: `title` is updated to accept a `React.Node` instead of string. <details> <summary>Expand for screenshot </summary>  </details> <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 <!-- Give as much information as needed to experiment with the component in the playground. --> - [Spin URL](https://admin.web.acc-menu.zakaria-warsame.us.spin.dev/store/shop1) - Notice how we're able to pass a custom activator with a squared avatar, and a different text style - The `title` "Test User" is also customized to use a different style. Passing just a string would still have the original style. ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide --------- Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
|
Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically. If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed. |
…ify#8953) <!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Resolves https://github.com/Shopify/core-workflows/issues/881 Follow up to: Shopify#8856 <!-- Context about the problem that’s being addressed. --> Part of [Commerce Components by Shopify](https://www.shopify.com/commerce-components); which requires some customization to the user menu ### WHAT is this pull request doing? `Topbar.UserMenu`: Allow a custom `activatorContent` prop to be passed enabling us to customize the order of the avatar and heading. <details> <summary> Expand to see before/after </summary> | Before | After | | ------------ |:-------------:| |  |  | </details> `ActionList`: `title` is updated to accept a `React.Node` instead of string. <details> <summary>Expand for screenshot </summary>  </details> <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 <!-- Give as much information as needed to experiment with the component in the playground. --> - [Spin URL](https://admin.web.acc-menu.zakaria-warsame.us.spin.dev/store/shop1) - Notice how we're able to pass a custom activator with a squared avatar, and a different text style - The `title` "Test User" is also customized to use a different style. Passing just a string would still have the original style. ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide --------- Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
WHY are these changes introduced?
Resolves https://github.com/Shopify/core-workflows/issues/881
Before the launch of Commerce Components by Shopify, there are a couple of prerequisites that require changes to Polaris. This PR addresses these changes.
I'm including screenshots of individual changes below but for in-depth specifications, please see this Figma.
WHAT is this pull request doing?
This PR updates the
Topbar.UserMenucomponent as well as theActionListto reflect the new store switcher experience.The changes being made here are three fold:
Topbar.UserMenu: Allow a customactivatorContentprop to be passed enabling us to customize the order of the avatar and heading. Our activator will have a square shaped avatar as well as an updated Text. Otherwise, the default activator stays the same (It might be a good idea to have the new format as the default, in which case I can update the existing activator).ActionList:titleis updated to accept aReact.Nodeinstead of string. Additionally a newhelpTextprop is added to accommodate email or other help texts.ActionList: the blue indicator that displays on active actions is removed. If there are any accessibility concerns regarding this, I'm happy to discuss.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
title"Test User" is also customized to use a different style. Passing just a string would still have the original style.Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes