-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[BulkActions] Bring everything up to the top #11622
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
5329b39 to
c22ed13
Compare
c22ed13 to
fdfbed4
Compare
|
/snapit |
|
✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221120537yarn add @shopify/polaris@0.0.0-snapshot-20240221120537 |
fdfbed4 to
caac7f7
Compare
|
/snapit |
|
✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221151805yarn add @shopify/polaris@0.0.0-snapshot-20240221151805 |
|
/snapit |
|
✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221154546yarn add @shopify/polaris@0.0.0-snapshot-20240221154546 |
|
/snapit |
|
✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221162416yarn add @shopify/polaris@0.0.0-snapshot-20240221162416 |
07b1cbb to
fc6b517
Compare
|
/snapit |
|
✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240221165828yarn add @shopify/polaris@0.0.0-snapshot-20240221165828 |
|
/snapit |
|
🫰✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240222104505yarn add @shopify/polaris@0.0.0-snapshot-20240222104505 |
adc0a22 to
6f2a570
Compare
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 items that roll-up into the overflow, a couple changes needed...
-
keep all "promoted actions" in the same section — no divider line between promoted action, however mainting divider line between actions that already existed in the overflow and the promoted actions that roll-up into the overflow
-
maintain order of items that get rolled up into overflow — ie: original order of items in header area
Bulk edit/Set as draft/Set as active— this should translate to this in the overflow menu... -
Bulk edit -
Set as draft -
Set as active -
Draft order bulk select position is broken as well as label is lost (
Actionsas there are none promoted). Let's ensure this isn't happening elsewhere as well.
|
Hi @mrcthms , thanks for the change 🙏 I just tried on your latest snapshot. After commenting out |
|
@boyutf I haven't made a snapshot with the changes to work without actions/promotedActions, I'll run the command now to create a new snapshot which should include those changes |
|
/snapit |
|
🫰✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240222165828yarn add @shopify/polaris@0.0.0-snapshot-20240222165828 |
|
/snapit |
|
🫰✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-20240222181016yarn add @shopify/polaris@0.0.0-snapshot-20240222181016 |
laurkim
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.
Looks good from code standpoint 💯
I have some concerns about the updates to the micro Button styles but most likely just need a bit more clarification on it.
| disclosureWidth, | ||
| promotedActions, | ||
| actionsWidths, | ||
| // setState, |
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.
🔥
|
|
||
| return <BulkActionsInner {...props} i18n={i18n} innerRef={ref} />; | ||
| return ( | ||
| <div className={bulkActionsClassNames} style={width ? {width} : undefined}> |
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={bulkActionsClassNames} style={width ? {width} : undefined}> | |
| <div className={styles.BulkActions} style={width ? {width} : undefined}> |
Light suggestion to just use styles.BulkActions directly here and remove bulkActionsClassNames variable higher up since classnames aren't being combined.
| &.variantMonochromePlain { | ||
| font-weight: var(--p-font-weight-medium); | ||
| font-size: var(--p-font-size-300); | ||
| } |
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.
From what I can tell, the BulkActionButton is rendering a Button without setting a variant (so it's using the default secondary variant). I might be missing another case where a monochromePlain is being rendered with bulk actions but if not, is this still needed?
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.
@laurkim Nope, not needed anymore! We have initial designs which used a micro monochrome plain, but have since updated it to be the regular micro button, so I'll remove this 🙌
| } as React.CSSProperties | ||
| } | ||
| > | ||
| <div className={paginationWrapperClassNames}> |
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={paginationWrapperClassNames}> | |
| <div className={styles.PaginationWrapper}> |
Light suggestion to use class name directly since multiple class names aren't being combined. Would require removing the paginationWrapperClassNames variable.
| <div className={styles.IndexTable}> | ||
| <div className={tableWrapperClassNames} ref={tableMeasurerRef}> | ||
| {!shouldShowBulkActions && !condensed && loadingMarkup} | ||
| <div className={tableWrapperClassNames}> |
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={tableWrapperClassNames}> | |
| <div className={styles.IndexTableWrapper}> |
Light suggestion to use class name directly since multiple class names aren't being combined. Would require removing the tableWrapperClassNames variable.
| } as React.CSSProperties | ||
| } | ||
| > | ||
| <div className={paginationWrapperClassNames}> |
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={paginationWrapperClassNames}> | |
| <div className={styles.PaginationWrapper}> |
| <ResourceListContext.Provider value={context}> | ||
| {filterControlMarkup} | ||
| <div className={resourceListWrapperClasses} ref={tableMeasurerRef}> | ||
| <div className={resourceListWrapperClasses}> |
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={resourceListWrapperClasses}> | |
| <div className={styles.ResourceListWrapper}> |
ouellettejordan
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.
Tested and all looks and works as expected 👍
### WHY are these changes introduced? Addresses Shopify/web#119227 Reliant on #11622 merging before this can merge. Updates the table variant of the Pagination component to match the new required UI. [Figma for reference](https://www.figma.com/file/shC6hyM1MC60abgslhpUib/Index-bulk-actions?type=design&node-id=11-207471&mode=design&t=alwlAtMB43g2Kk1h-0). ### WHAT is this pull request doing? - Centre-align buttons - Add prominence to buttons with background color that's different to surrounding box. ### How to 🎩 Spin URL: https://admin.web.pagination-refresh.marc-thomas.eu.spin.dev/store/shop1/products?selectedView=all ### 🎩 checklist - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [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
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@12.19.0 ### Minor Changes - [#11399](#11399) [`0134d2378`](0134d23) Thanks [@sophschneider](https://github.com/sophschneider)! - Added `PositionedOverlay` scroll support for all scroll containers - [#11622](#11622) [`1f81501c8`](1f81501) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated the BulkActions component to include logic to handling selecting and deselecting rows - [#11637](#11637) [`1ac638246`](1ac6382) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated Pagination table variant to have more prominent and centrally-aligned actions ### Patch Changes - [#11644](#11644) [`b95fc9807`](b95fc98) Thanks [@kyledurand](https://github.com/kyledurand)! - Removed nav wrapper from breadcrumbs since it now only renders a single link ## polaris.shopify.com@0.63.2 ### Patch Changes - Updated dependencies \[[`0134d2378`](0134d23), [`1f81501c8`](1f81501), [`1ac638246`](1ac6382), [`b95fc9807`](b95fc98)]: - @shopify/polaris@12.19.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
Guys seriously make up your mind which direction you are going with this. This is the 3rd major change in recent weeks. The old method of rendering the bulk actions menu over the bottom of the table was the best. Previous version was okay but had issues with CLS and now this new way requires the user to move the mouse pointer to the opposite side of the screen to select the action required. Not a great user experience IMO. |
### WHY are these changes introduced? Fixes Shopify/web#119088 We want to further update the bulk/select actions to re-combine them into the same piece of UI. We had broken bulk and select actions up as part of the initial Index Filtering and Search work, but now realise that keeping them colocated is the best UX for our merchants. ### WHAT is this pull request doing? <img width="1624" alt="Screenshot 2024-02-22 at 12 23 50" src="https://github.com/Shopify/polaris/assets/2562596/11b33a11-474a-42fb-8475-1137da228481"> This PR: - re-introduces all the select logic to the BulkActions component - left-align select actions, right-align bulk actions - rewrites the BulkActions component to a function component from a class component - updates the logic to show/hide promoted actions, as the previous method was broken - deprecate the SelectAllActions component - updates IndexTable & ResourceList to remove SelectAllActions, update BulkActions implementation - removes the useIsSelectAllActionsSticky hook (never exported, only used to position the SelectAllActions internally within ResourceList/IndexTable The biggest piece of work is updating how the bulk actions show/rollup logic works. Beforehand, it would either show all promoted actions if they fit, or roll every single one up behind a meatball menu. This wasn't great, as we should show as many as possible in the available space. As part of the update to a function component, we now utilise a similar layout sizing mechanism as we use in both the Tabs and ActionMenu -> Actions components, to render as many promoted actions as we possibly can in the available space. ### How to 🎩 Spin URL for IndexTable: https://admin.web.bulk-ui-refresh.marc-thomas.eu.spin.dev/store/shop1/products?selectedView=all Spin URL for ResourceList: https://admin.web.bulk-ui-refresh.marc-thomas.eu.spin.dev/store/shop1/products/32 Spin URL for Updated Draft orders (using Shopify/web `ResourceListWithHeader`): https://admin.web.bulk-ui-refresh.marc-thomas.eu.spin.dev/store/shop1/draft_orders?selectedView=all ### 🎩 checklist - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [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

WHY are these changes introduced?
Fixes https://github.com/Shopify/web/issues/119088
We want to further update the bulk/select actions to re-combine them into the same piece of UI. We had broken bulk and select actions up as part of the initial Index Filtering and Search work, but now realise that keeping them colocated is the best UX for our merchants.
WHAT is this pull request doing?
This PR:
The biggest piece of work is updating how the bulk actions show/rollup logic works. Beforehand, it would either show all promoted actions if they fit, or roll every single one up behind a meatball menu. This wasn't great, as we should show as many as possible in the available space. As part of the update to a function component, we now utilise a similar layout sizing mechanism as we use in both the Tabs and ActionMenu -> Actions components, to render as many promoted actions as we possibly can in the available space.
How to 🎩
Spin URL for IndexTable: https://admin.web.bulk-ui-refresh.marc-thomas.eu.spin.dev/store/shop1/products?selectedView=all
Spin URL for ResourceList: https://admin.web.bulk-ui-refresh.marc-thomas.eu.spin.dev/store/shop1/products/32
Spin URL for Updated Draft orders (using Shopify/web
ResourceListWithHeader): https://admin.web.bulk-ui-refresh.marc-thomas.eu.spin.dev/store/shop1/draft_orders?selectedView=all🎩 checklist
README.mdwith documentation changes