Clean up menu item view code and re-render when menu items change.

Review Request #13752 — Created April 18, 2024 and submitted

Information

Ink
master

Reviewers

The MenuView component would happily re-render itself when the list of
menu items changed, but the menu items themselves didn't reflect changes
to the MenuItem attributes. This means that if a label or icon
changed, the menu would have to be explicitly re-rendered to reflect it,
and that operation could be relatively heavy.

Now, attributes will re-render part of or the entirety of the specific
menu item that changed.

To faciliate this, the code for building menu items has changed to
instantiate views specific to each menu item type. This lets us contain
the render logic, DOM updating, and event handling in individual views,
simplifying both those code paths and MenuView itself.

This work also made clear that we had some state management issues. We
weren't unregistering keyboard shortcuts when the menu items were
replaced, causing warnings. We're now able to make use of the full
BaseView.remove() logic to tear down each menu item properly when
replacing menu items or tearing down a MenuView.

One additional but related change is that the onClick() handlers for
menu items now take a MenuItem parameter as the first parameter,
giving handlers the ability to easily manipulate or access state on the
appropriate menu item.

Unit tests pass.

Tested that I could dynamically change state on menu items and that
they re-rendered correctly.

Summary ID
Clean up menu item view code and re-render when menu items change.
The `MenuView` component would happily re-render itself when the list of menu items changed, but the menu items themselves didn't reflect changes to the `MenuItem` attributes. This means that if a label or icon changed, the menu would have to be explicitly re-rendered to reflect it, and that operation could be relatively heavy. Now, attributes will re-render part of or the entirety of the specific menu item that changed. To faciliate this, the code for building menu items has changed to instantiate views specific to each menu item type. This lets us contain the render logic, DOM updating, and event handling in individual views, simplifying both those code paths and `MenuView` itself. This work also made clear that we had some state management issues. We weren't unregistering keyboard shortcuts when the menu items were replaced, causing warnings. We're now able to make use of the full `BaseView.remove()` logic to tear down each menu item properly when replacing menu items or tearing down a `MenuView`. One additional but related change is that the `onClick()` handlers for menu items now take a `MenuItem` parameter as the first parameter, giving handlers the ability to easily manipulate or access state on the appropriate menu item.
1e4b8c01617080fe6a0a9b7ccbb9109b03e28958
david
  1. Ship It!
  2. 
      
maubin
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to master (37bc5bb)