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: Closed (submitted)

Change Summary:

Pushed to release-7.x (620efc3)
Loading...