-
Notifications
You must be signed in to change notification settings - Fork 8
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
Create HLD for action menus in the table #956
Merged
Merged
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
2fb67cc
first pass at spec
mollykreis e044fe1
minor updates
mollykreis 5405d52
format
mollykreis e78f54d
Change files
mollykreis c4c021b
Merge branch 'main' into table-action-menu-spec
mollykreis 5a66420
minor updates
mollykreis 8a25c65
Merge branch 'main' into table-action-menu-spec
mollykreis e248926
Update packages/nimble-components/src/table/specs/action-menu-hld.md
mollykreis 9e1454c
Update packages/nimble-components/src/table/specs/action-menu-hld.md
mollykreis ae0c5a0
Merge branch 'main' into table-action-menu-spec
mollykreis e41b887
partial update
mollykreis f855821
more updates
mollykreis 47af5dd
note about handling menu events
mollykreis 5b301ed
format
mollykreis 704cb4c
rename events
mollykreis 508570f
format
mollykreis 3ba6327
Merge branch 'main' into table-action-menu-spec
mollykreis a252322
Merge branch 'main' into table-action-menu-spec
mollykreis 8971628
Update packages/nimble-components/src/table/specs/action-menu-hld.md
mollykreis 2e42901
PR feedback
mollykreis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
change/@ni-nimble-components-3ea9babf-8c16-449e-9f7f-611b902b5045.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "none", | ||
"comment": "HLD for table's action menu", | ||
"packageName": "@ni/nimble-components", | ||
"email": "[email protected]", | ||
"dependentChangeType": "none" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
113 changes: 113 additions & 0 deletions
113
packages/nimble-components/src/table/specs/action-menu-hld.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
# Nimble Table Action Menu API | ||
|
||
## Problem Statement | ||
|
||
In some tables, such as many tables within SLE, there is a need to put a menu button within a row of the table. The `nimble-table` should provide a way for clients to accomplish this to avoid code duplication and promote consistency. | ||
|
||
### Out of scope of this HLD | ||
|
||
- The interaction of the menu opening and closing in coordination with row selection is out of scope for this HLD. This interaction will be discussed when row selection is defined. | ||
|
||
## Links To Relevant Work Items and Reference Material | ||
|
||
[Table Spec](./README.md) | ||
|
||
Example action menu in SLE | ||
![SLE action menu](./spec-images/sleActionMenu.png) | ||
|
||
## Implementation / Design | ||
|
||
### Client-facing API | ||
|
||
There are two pieces of configuration that must be provided by a client in order to use the action menu: | ||
|
||
1. Specifying which column, or columns, the action menu will be visible within | ||
2. Specifying the menu and its items to show when the menu button is open | ||
|
||
The nimble design system will be opinionated about many details of the menu button within a table. Therefore, a client will not be able to configure the exact details of the menu button itself, such as: | ||
|
||
- The appearance mode of the button | ||
- The icon shown within the button | ||
- When the button becomes visible (e.g. always visible, visible on hover/selection only, etc.) | ||
|
||
If the need arises for a client to have more control of the menu button's configuration, the API can be extended at a later time. | ||
mollykreis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### Specifying a column to include a menu button | ||
|
||
A client can specify that a column should have an action menu within it by adding the `show-menu` attribute on the column definition. In the example below, the menu will be added to the _First name_ column only. The `show-menu` attribute can be added to multiple columns if multiple action menus are desired within a row. | ||
|
||
```HTML | ||
<nimble-table> | ||
<nimble-table-column-text field-name="firstName" show-menu>First name</nimble-table-column-text> | ||
<nimble-table-column-text field-name="lastName">Last name</nimble-table-column-text> | ||
</nimble-table> | ||
``` | ||
|
||
### Providing a menu | ||
|
||
Because only one menu can be open on the page at a time, the client can provide a single menu that will be associated with the correct menu button when it is open. The menu is provided by the client by slotting a menu in the `action-menu` slot as shown below: | ||
|
||
```HTML | ||
<nimble-table> | ||
<nimble-table-column-text field-name="firstName" show-menu>First name</nimble-table-column-text> | ||
<nimble-table-column-text field-name="lastName">Last name</nimble-table-column-text> | ||
|
||
<nimble-menu slot="action-menu"> | ||
<nimble-menu-item>My first action</nimble-menu-item> | ||
<nimble-menu-item>My second action</nimble-menu-item> | ||
<nimble-menu-item>My last action</nimble-menu-item> | ||
</nimble-menu> | ||
</nimble-table> | ||
``` | ||
|
||
If an application requires different menu items for different rows, the client is responsible for ensuring that the items in the menu are correct for the row(s) that the menu is associated. This can be done by handling the `action-menu-opening` event on the table and updating the menu items as appropriate. The `action-menu-opening` event will include the following in its details: | ||
mollykreis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- `rowIds` - string array - The IDs of the rows that the menu is associated with. | ||
- `columnId` - string, possibly undefined - The ID of the column that the menu is associated with. A column ID is optional on a column definition. If the menu is associated with a column without an ID, `columnId` will be `undefined` in the event details. | ||
|
||
mollykreis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
### Implementation | ||
|
||
#### Slot Forwarding | ||
|
||
In order for the menu slotted within the table to be slotted within a cell's `nimble-menu-button`, the menu needs to be "forwarded" from the table to a row and then to a cell. Every row will have a slot for an menu, but only the row with the open menu will have something slotted within it. Similarly, every cell in a column with a menu will have a slot for a menu, but only the cell with an open menu will have something slotted within it. Events will be used to know when the row and cell the menu is associated with needs to change, and the templates will dynamically rename their slots to ensure the menu is slotted in the correct elements. | ||
mollykreis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For example, the table's template will look something like this: | ||
|
||
```HTML | ||
<template> | ||
... | ||
<nimble-table-row> | ||
<slot slot="row-action-menu" name="${(x, c) => ((c.parent.openActionMenuRowId === x.id) ? 'action-menu' : 'unused-action-menu')}"></slot> | ||
</nimble-table-row> | ||
... | ||
</template> | ||
``` | ||
|
||
#### Updates to `nimble-menu-button` | ||
|
||
To implement the design described above, a few small changes need to be made to the existing `nimble-menu-button`: | ||
|
||
- Add an `opening` event that gets fired immediately before the menu is opened. This new event will allow the table to slot the menu into the correct row and cell prior to the menu actually opening. This is important to ensure that the menu items can be focused correctly upon the menu opening. | ||
mollykreis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Update the code that gets the slotted menu. Currently, the code only looks for the first element in the `menu` slot. With the design above, the menu element will be nested within a few `slot` elements, so the code will be updated to handle both DOM structures. | ||
|
||
### Framework Integration | ||
|
||
There is no framework-specific integration work necessary with this design. One large benefit of the approach to slot a menu within the table is that applications written using frameworks can use framework-specific mechanisms to build the menu/menu-items and handle events associated with the menu. | ||
|
||
## Alternative Implementations / Designs | ||
|
||
### Provide column IDs to the table to specify columns with menus | ||
|
||
Rather than configuring a column to have a menu by adding an attribute to the column definition, a column could be configured to have a menu by adding a property to the table that is the array of column IDs that should have a menu. This is not ideal for a few different reasons: | ||
|
||
- It requires columns to have IDs when this may not otherwise be necessary. | ||
- It is more error prone because mistakes could be made keeping the configuration of column IDs in sync between the column definitions and table configuration. | ||
|
||
## Open Issues | ||
|
||
- Can the action menu be opened for multiple rows at the same time? This doesn't become possible until the table supports row selection, but it can impact the API. To be the most future-proof, the API is designed such that the `action-menu-opening` event includes an array of row IDs rather than a single row ID string. | ||
- The details of the visual/interaction design still need to be finalized by the designers, but these details should have minimal impact on the implementation. Some items that need to be finalized are: | ||
- What icon will be used for the menu? Will it still be the three dots in a horizontal line? | ||
- What is the exact interaction with the menu being visible on hover? | ||
- Is the space of the menu-button always reserved within a column, or does the space collapse when the button is hidden? | ||
- How does the menu interact with row selection? This is a future-looking question because the table does not currently support selection. |
Binary file added
BIN
+36.4 KB
packages/nimble-components/src/table/specs/spec-images/sleActionMenu.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Html spec for popovers just got approved! The event for "I'm about to open but haven't opened yet" is "popovershow": https://open-ui.org/components/popup.research.explainer#events
So for our custom event I'd keep the hyphens but suggest we go
action-menu-show
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.
Discussion on why popover naming differs from dialog openui/open-ui#322
From a method point of view (dialog does not have a "show" type event while popover does)
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'm guessing this naming convention should also be applied to the
nimble-menu-button
? Should the new event I'm proposing to add on that component beshow
rather thanopening
? It already has an event namedopen-change
that fires right after the menu is opened or closed. Do you think there is a better name for the existing event? I don't see an event in the popup spec that aligns with our existing event. It would be a breaking change, but I don't think there are any usages of it (at least in SLE).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.
turns out the popover merge jumped the gun, was reverted, and reopened π
Also the explainer I linked seems to be out of date with the current changes, it looks like the current idea (most likely to merge I think) is a single toggle event instead of separate show/hide.
That seems more in line with the "open-change" on menu-button.
(haven't fully responded to your qs yet, just posting some updates)
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 like it's settling on
beforetoggle
andtoggle
withnewState
andoldState
in details: whatwg/html#8717 (comment)Wanna copy
beforetoggle
for the menu-button? Seems like reasonable alignment (since eventually we will probably use that feature).Guess we still want to scope the name in table since there will be other "toggley" things happening, so
action-menu-beforetoggle
?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.
beforetoggle
on the menu-button andaction-menu-beforetoggle
seem like reasonable names to me. I assume that means we'd also want to fire the events right before they close too. Is that your thought as well? I'm guessing most clients will only care about the menu opening, but it seems odd to have abeforetoggle
event that only fires when toggling in one direction (i.e. from closed to open).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.
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've updated all the table-related specs. I plan to update the README for the menu-button when I make the changes to that component.