• 
      

    Integrate Review Board Actions with the new Ink.MenuView component.

    Review Request #13753 — Created April 19, 2024 and submitted

    Information

    Review Board
    release-7.x

    Reviewers

    Review Board 6 introduced a formal foundation for actions, used in the
    page top bar and the review request actions bar. When this was
    implemented, there wasn't really a solid formal way of managing most
    elements of a menu item, or managing state around menu items, so the
    actions needed to do a lot of custom rendering.

    This is no longer the case, as we have a fairly fleshed-out menu
    implementation in Ink.MenuView. This came too late to influence the
    action design, though.

    This change works to bridge the gap. More information on actions are now
    provided in the model data. MenuActionView then uses this to better
    construct menu items with the correct properties. This helps ensure that
    icons in, for example, the Follow menu are in the right place, and that
    links vs. onClick handlers work correctly.

    We do still need to render some custom menu items, notably in the Review
    menu, so we still need a mechanism for custom rendering. This is being
    done by introducing an is_custom_rendered() method in the Python
    action code. By default, this checks if the action uses a custom
    Django template, but subclasses can explicitly implement this to return
    a value. If we're in this path, we ignore the menu icon, label, and URL
    for the menu item, and leave it entirely up to the view.

    Some actions received updates to better work with this. In particular,
    the archive/mute actions no longer reach into the internals of the
    element to change the label, but rather set an attribute on the
    MenuItem model, causing a render change in Ink.

    Going forward, we'll want to work more toward giving the components
    managing an action more control over the rendering, rather than giving
    the action control itself. This isn't applicable to our code at this
    time, but more something to consider as we evolve and document actions.

    One additional note is that this partially reverts a change recently
    made to BaseVisibilityActionView. I had addressed a TypeScript
    compilation error where the abstract class could not be used with
    @spina. This is still true, but the fix was wrong. By making a class
    abstract in the middle of a Spina object hierarchy, we don't get any of
    Spina's property management (such as static-to-prototype). Until we have
    a method for making decorators and abstract classes place nice, abstract
    classes in Spina hierarchies are not safe to use in our codebase.

    Tested every action we provide, making sure they worked as expected.

    Python and JavaScript unit tests pass (except for ones that were already
    failing).

    Summary ID
    Integrate Review Board Actions with the new Ink.MenuView component.
    Review Board 6 introduced a formal foundation for actions, used in the page top bar and the review request actions bar. When this was implemented, there wasn't really a solid formal way of managing most elements of a menu item, or managing state around menu items, so the actions needed to do a lot of custom rendering. This is no longer the case, as we have a fairly fleshed-out menu implementation in `Ink.MenuView`. This came too late to influence the action design, though. This change works to bridge the gap. More information on actions are now provided in the model data. `MenuActionView` then uses this to better construct menu items with the correct properties. This helps ensure that icons in, for example, the Follow menu are in the right place, and that links vs. `onClick` handlers work correctly. We do still need to render some custom menu items, notably in the Review menu, so we still need a mechanism for custom rendering. This is being done by introducing an `is_custom_rendered()` method in the Python action code. By default, this checks if the action uses a custom Django template, but subclasses can explicitly implement this to return a value. If we're in this path, we ignore the menu icon, label, and URL for the menu item, and leave it entirely up to the view. Some actions received updates to better work with this. In particular, the archive/mute actions no longer reach into the internals of the element to change the label, but rather set an attribute on the `MenuItem` model, causing a render change in Ink. Going forward, we'll want to work more toward giving the components managing an action more control over the rendering, rather than giving the action control itself. This isn't applicable to our code at this time, but more something to consider as we evolve and document actions. One additional note is that this partially reverts a change recently made to `BaseVisibilityActionView`. I had addressed a TypeScript compilation error where the `abstract` class could not be used with `@spina`. This is still true, but the fix was wrong. By making a class abstract in the middle of a Spina object hierarchy, we don't get any of Spina's property management (such as static-to-prototype). Until we have a method for making decorators and abstract classes place nice, abstract classes in Spina hierarchies are not safe to use in our codebase.
    9f7aeb6264deede76c63fe3a25dbd8b43d94869a

    david
    1. Ship It!
    2. 
        
    maubin
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-7.x (620efc3)