-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat(Table) : adding context menu for table row to example and prototype #12253
feat(Table) : adding context menu for table row to example and prototype #12253
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: fd45f9cd4a8649a2f39f5af5829a91ce6c649f00 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
@@ -10,6 +19,10 @@ const InteractiveTable = () => { | |||
setPopupOpen(false); | |||
}; | |||
|
|||
const contextMenuItems = ['Add to selection', 'Remove', 'Download']; | |||
|
|||
const getRowWithContextMenu = tableRow => <MenuButton contextMenu trigger={tableRow} menu={contextMenuItems} />; |
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 would rename this to just withContextMenu
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.
done :)
@@ -54,7 +56,8 @@ const rowsPlain = [ | |||
moreOptionCell | |||
], | |||
onClick: () => handleRowClick(1), | |||
'aria-labelledby': 'name-1 age-1' | |||
'aria-labelledby': 'name-1 age-1', | |||
children: (Component, props) => <MenuButton menu={contextMenuItems} contextMenu trigger={<Component {...props} />} /> |
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 think that this will produce a key
error in console as key
will be still on a row. The change below will move it to a proper component.
children: (Component, props) => <MenuButton menu={contextMenuItems} contextMenu trigger={<Component {...props} />} /> | |
children: (Component, { key, ...rest }) => <MenuButton menu={contextMenuItems} key={key} contextMenu trigger={<Component {...rest} />} /> |
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.
fixed :)
</Table.Row> | ||
{withContextMenu( | ||
<Table.Row key="1" accessibility={gridRowBehavior}> | ||
<Table.Cell content="1" key="1-1" accessibility={gridCellBehavior} /> |
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.
<Table.Cell content="1" key="1-1" accessibility={gridCellBehavior} /> | |
<Table.Cell content="1" accessibility={gridCellBehavior} /> |
key
here and on all other Table.Cell
and Table.Roe
is redundant
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.
fixed :)
<Table.Cell content="30000000000000 years" key="1-4" accessibility={gridCellBehavior} /> | ||
</Table.Row> | ||
{withContextMenu( | ||
<Table.Row key="1" accessibility={gridRowBehavior}> |
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.
<Table.Row key="1" accessibility={gridRowBehavior}> | |
<Table.Row accessibility={gridRowBehavior}> |
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.
fixed :)
moreActionCell, | ||
moreOptionCell | ||
moreActionCell('1-5'), | ||
moreOptionCell('1-6') |
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 would prefer an approach with spread:
moreOptionCell('1-6') | |
{ key: '1-6', ...moreOptionCell }, |
But it's not blocking
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.
fixed :)
Pull request checklist
Description of changes
Adding usage of context menu for table row.
Focus areas to test
Added into:
Microsoft Reviewers: Open in CodeFlow