• 
      

    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)