-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[BulkActions] - Allowing promoted actions to be rendered as menus #4266
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
size-limit report
|
f9f7fb3 to
1b80584
Compare
|
cc: @sarahill |
AndrewMusgrave
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 really good! 💯
| function instanceOfMenuGroupDescriptor( | ||
| action: MenuGroupDescriptor | BulkAction, | ||
| ): action is MenuGroupDescriptor { | ||
| return (action as MenuGroupDescriptor).title !== 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.
If you wanted a type safe way (without casting), we can use the in operator
return 'title' in action
| isNewBadgeInBadgeActions: boolean; | ||
| } | ||
|
|
||
| export function BulkActionMenu({ |
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.
It shouldn't need a lot, but would you mind adding a few tests 🙏
| actions, | ||
| isNewBadgeInBadgeActions, | ||
| }: BulkActionsMenuProps) { | ||
| const [isVisible, setVisible] = useState<boolean>(false); |
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.
Small nit: we have a useToggle hook
| expect(bulkActionButtons).toHaveLength(4); | ||
| const bulkActionMenus = bulkActions.find(BulkActionMenu); | ||
| expect(bulkActionMenus).toHaveLength(2); | ||
| warnSpy.mockRestore(); |
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 know this pattern was already used in this file, but should we move this + spyOn to a beforeEach/afterEach?
f1d1499 to
d341b88
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.
a7ee53a to
b00d26e
Compare
iangermann
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.
Code lgtm! @AndrewMusgrave do you think it would make sense to add an example of this to the IndexTable storybook?
AndrewMusgrave
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.
LGTM ✅ @Germanika a new example totally makes sense!!
b00d26e to
5d99b39
Compare
5d99b39 to
7b28e57
Compare
|
/shipit |

WHY are these changes introduced?
Closes #4246
Currently, promoted actions are single buttons and are limited to only 2 actions in the
BulkActioncomponent. With this change, we're proposing that each one of these buttons can become a menu where we would group relevant actions by subject. This can be seen on this card with the real use case where it would be first used.WHAT is this pull request doing?
What it looks like now 🎩
On large screensJun-17-2021.04-07-28.mp4
To make this possible, we're basically changing the
promotedActionsprop to also accept values of typeMenuGroupDescriptor, which are essentially the type of the menus that we'll be rendering. Then where we define what thepromotedActionsmarkup will look like, we're mapping through the actions, checking for their type through a type guard function and deciding based on it whether we should render aBulkActionButtonor aBulkActionMenu(which is also being introduced in this PR).The other change introduced on this PR was for smaller screens, where promoted actions will get rolled into a general
Actionsmenu. Here we still want the actions on the menus to be grouped together, so we're making sure they work asActionListSection, similarly to how the propactionsis being treated.Notes
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes (there's no readme for this component)