Create HLD for action menus in the table #956
Conversation
mollykreis
requested review from
rajsite,
jattasNI and
atmgrifter00
as code owners
| @@ -151,14 +149,16 @@ _Functions_ | |||
|
|
|||
| _Events_ | |||
|
|
|||
| Placeholder | |||
| - `action-menu-opening` - An event that is emitted immediately prior to the action menu opening. This can be used to update the items in the menu so that they are in the correct state for the row(s) the menu is associated with. The event details include the following: | |||
There was a problem hiding this comment.
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.
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.
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 be show rather than opening? It already has an event named open-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.
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.
Looks like it's settling on beforetoggle and toggle with newState and oldState 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.
beforetoggle on the menu-button and action-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 a beforetoggle event that only fires when toggling in one direction (i.e. from closed to open).
There was a problem hiding this comment.
assume that means we'd also want to fire the events right before they close too
yep! makes sense to do it in both toggle directions 👍
There was a problem hiding this comment.
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.
Co-authored-by: Jesse Attas <[email protected]>
Co-authored-by: Jesse Attas <[email protected]>
packages/nimble-components/src/table/specs/table-columns-hld.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Jesse Attas <[email protected]>
# Pull Request ## 🤨 Rationale Some changes need to be made to the menu button before it can be used within the table for the action menu. Specifically, those changes are: - adding a `beforetoggle` event - having the menu button find the slotted menu when it is nested within multiple slots See #956 for more details. ## 👩💻 Implementation - Add `beforetoggle` event to the menu button that is fired before the menu opens or closes - Add Angular & Blazor support for the `beforetoggle` event - Change the logic for finding the slotted menu. We can no longer use a filter in the template to find slotted elements with a role of `menu` because this won't match `slot` elements. Instead, the template assigns all slotted elements to the `slottedMenus` array, and the getter for `menu` finds the menu, which may be nested within `slot` elements. ## 🧪 Testing - Manually tested that the `beforetoggle` event handler works correctly in Blazor & Angular - Added auto tests for the `beforetoggle` event works correctly for the web components - Basic manual testing in Storybook that the component still behaves as expected - Added tests that the focus & menu opening behavior works correctly when the menu is nested in an additional slot ## ✅ Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed.
Pull Request
🤨 Rationale
Create a spec for #859
👩💻 Implementation
N/A
🧪 Testing
N/A
✅ Checklist