fix: add documented events to menu-button #219 #232
Conversation
There was a problem hiding this comment.
Pull request overview
Adds documented menu-button-level events for select/change, aligning event listeners with the menu-button widget rather than the underlying menu element.
Changes:
- Adds a dedicated handler for
"makeup-menu-change"and dispatches"makeup-menu-button-change". - Dispatches
"makeup-menu-button-select"on select and refactors collapse/focus logic into a shared helper. - Updates docs example to listen on
widget.elfor the documented menu-button events.
Reviewed changes
Copilot reviewed 2 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ui/makeup-menu-button/src/index.js | Splits change vs select handling, dispatches documented menu-button events, and centralizes collapse behavior. |
| docs/ui/makeup-menu-button/index.js | Updates example listeners to the new menu-button event targets/types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.menu.el.removeEventListener("makeup-menu-change", this._onMenuItemChangeListener); | ||
| } | ||
|
|
||
| // this should maybe be moved to expander as an option callback for after collapse |
There was a problem hiding this comment.
This comment is speculative and not actionable as-is. Consider converting it into a tracked TODO (ideally referencing an issue) or removing it once a decision is made, so the codebase doesn’t accumulate ambiguous guidance.
| // this should maybe be moved to expander as an option callback for after collapse | |
| // TODO: Move this logic to Expander as an option callback that runs after collapse. |
| _collapseMenuAfterTimeout() { | ||
| const widget = this; | ||
|
|
||
| setTimeout(function () { | ||
| widget._expander.expanded = false; | ||
| widget._buttonEl.focus(); | ||
| }, 150); | ||
| } |
There was a problem hiding this comment.
The timeout duration 150 is a magic number and the const widget = this pattern makes the helper harder to read/maintain. Consider extracting the delay into a named constant (e.g., COLLAPSE_DELAY_MS) and using an arrow function in setTimeout so this can be used directly.
|
At first, I wondered why the menu collapses on change. Looking closer, |
2 participants
Fixes #219