Add back rowLinkTo and handleRowClick behaviors

[martha]

Description

GH issue: https://github.com/open-path/Green-River/issues/6761
Based on our decision in slack (thread)

Summary of changes:

• Remove TableRowActions component's primaryActionConfig prop. You can still pass a primaryAction, which BulkServices does, but otherwise all of the action configs go in the dropdown menu and are now called menuActionConfigs
• Re-add rowLinkTo and handleRowClick on tables where they were removed in the base deploy branch
• Re-add rowLinkState prop to GenericTable
• Remove the column props linkTreatment and ariaLabel
  • linkTreatment goes against our updated pattern and should no longer be used (I'm open to removing its removal from this PR if you prefer, though, since there are some out-of-scope tables that still use it)
  • ariaLabel is not needed anymore, since row links are accessible through the row action menu and have accessible labels there.

Implementation notes for posterity

• I originally set about to use an onClick handler on the row for both handleRowClick and rowLinkTo, thinking this was desirable because it removed link markup from each cell in the table, making the table easier to understand when navigating via screenreader.
• However, predictably, removing this row markup has other undesirable side effects!
  • Can't ctrl-click to open in new tab
  • Can't hover over the row to see where the link will take you (chrome)
• I decided to keep our existing implementation, with a RouterLink in each cell. The screenreader does still now read "link" on each cell, but otherwise it reads the cell contents. The first menu item is accessibly marked up with more detail about where the link will take you. I think this is an OK tradeoff.

Type of change

• Bug fix
• New feature (adds functionality)
• Code clean-up / housekeeping
• Dependency update

Checklist before requesting review

• I have performed a self-review of my code
• I have run the code that is being changed under ideal conditions, and it doesn't fail
• My code includes comments and/or descriptive variable names to help other engineers understand the intent (or not applicable)
• My code follows the style guidelines of this project (eslint)
• I have updated the documentation (or not applicable)
• If it's not obvious how to test this change, I have provided testing instructions in this PR or the related issue

[gigxz]

Looks good @martha!

The keyboard navigation experience feels a little bit degraded because of the need to press tab twice to get to the next row (since you tab over to the menu icon). Maybe that's unavoidable, but just noting it, if you have any ideas. For example the Projects table, it seems particularly unnecessary since that menu isn't actually providing any additional nav functionality.

[gigxz]

I'm not seeing the outline visually when testing. This fixes it for me:

          outline: 'auto',
          outlineOffset: '-5px',

(5px so it doesn't get unevenly cut off, but maybe that's preferable to having it more inside the cell - could go either way

[martha]

Thanks! I think -5px is good.

outline: 'auto' for me makes the outline look black, so I also added an outlineColor that matches what we apply to buttons in our theme.ts. Interesting that this is treated differently in Chrome than Safari.

[martha]

@gigxz 100% agree about the keyboard nav experience :(. two ideas:

1. Make the action menu the focus target for each row, instead of the primary cell.
   • Pros: Cleaner code; experience is more similar across keyboard and screenreader
   • Cons: Keyboard users have to open the menu so there are still some extra steps.
2. IF there is only one menu item, make the 3-dots menu non-focusable (for keyboard only, not voiceover).
   • Pros: Nicer UX for keyboard users
   • Cons: Brittle because it relies on every table that uses Table Actions making sure that the first action passed to the action menu is the same as the click target for the whole row. (This isn't true in Bulk Services, so that would already require some special casing, and it's brittle to have to remember special casing in the future.) Another con is the dissimilar UX across tables that contain 1 versus more than 1 action in the menu. Seems like it would be nicer to keep that consistent.

Edit: Playing around with both of these, I prefer 1. The extra step is just that you have to press enter twice, so it doesn't seem that bad.

[gigxz]

@martha I like the tab nav experience! But I just noticed that we now lose any keyboard navigability for tables that use rowLinkTo without a TableRowActions column. (see Form definitions table, client recent enrollment, enrollment reminders).

Since the presence of the action column is affecting focus and linking behavior of the table as whole, maybe the answer is to move it back to a prop and remove it from the columns array. 🙈 I know you have already implemented it this way and there's some code churn. I don't see a nice alternative because the GenericTable really needs to know some information about the "Action column" in order to determine how to implement row linking. (We could tell it explicitly through another prop, but that feels redundant. Or infer it based on the columns prop, but that feels brittle). This would also reduce duplication between rowLinkTo/columns.

<GenericTable
    recordName={optional, gets passed along to TableRowActions

    renderMenuActionButton={row => ReactNode} // optional, gets passed along to TableRowActions. typically not a navigation action. bulk services uses

    // optional, renders each item in the three-dot menu. optionally specify performOnRowClick on max 1 item IF you want to perform this action when clicking the row. (if specified on multiple, raise)
    // all gets passed along to TableRowActions. we just look at performOnRowClick in generic table
    menuActionConfigs={row => [{ to: safePath, performOnRowClick: true}, {to: otherSafepath}, ... ] }
/>

What do you think? Please feel free to push back!
This is an important API to get right so we can have a better chance at consistency going forward when we are adding new tables, so think it's worth noodling on a bit more.

[martha]

Ooh, yeah thanks for catching that. I am open to this idea. Let's also confirm/gut-check: Does it seem worth it to implement vs. just reverting to this behavior from one iteration ago, where both the primary column AND the nav menu are keyboard accessible?

For consistency going forward, our desired pattern is to never use rowLinkTo without TableRowActions, for better accessibility. So, maybe we could design the API and implement GenericTable so that all tables with rowLinkTo and handleRowClick get an action menu by default? Something like,

<GenericTable
  recordName={''}
  rowLinkTo={''}
  handleRowClick={() => {}}
  // if either rowLinkTo or handleRowClick is provided, then render TableRowActions, regardless of whether additional secondaryActionConfigs have been passed
  secondaryActionConfigs={[{ to: safePath, ... }, {onClick: ...}, ... ] } // optional, only provided if there are additional menu items in here besides the primary one
  primaryAction={optional, only used for 'fancy' primary action, like bulk services}  // `primaryAction` maybe is mutually exclusive with `rowLinkTo` and `handleRowClick`? 
  // or maybe update the naming... I think of "primary action" as referring to the row onclick behavior
/>

What do you think?

[gigxz]

our desired pattern is to never use rowLinkTo without TableRowActions

This is true for the majority of the application's tables, but I don't think we should build ourselves out of simple clickable rows (ie a or onClick without a menu). Considering the mini-table-cards we have--like the To Do list and the Recent Enrollments card--those are a distinct kind of table with clickable rows that will likely remain as-is. (Design-wise, the menu treatment doesn't really work there.) So I think whichever way we go, we want to continue to support both. That could be done by adding a hideMenu prop or something, though.

If we just default to showing the menu instead of specifying it using a CommonMenuItem config, how would we override the title/ariaLabel/pass link state? Everything would need to be passed as additional props, I think? Say for example we want the case note row have primary action text View Note or Edit Note etc, right now I believe the title is passed in the common menu item object.

[martha]

I think I could go either way. I made a start on implementing this and was happy with how much code cruft we could delete, which makes me feel biased towards the hideMenu prop idea. But maybe it is a little convoluted.

You are right about the additional props. I was thinking of trying to reuse the recordType prop and pass it down from GenericTableWithData to GenericTable. But it wouldn't work for "Edit" instead of "View." (And doesn't apply at all for rowLinkState, openInNew, etc.) I'm ok with adding an extra prop for all the attributes needed. Or we could go in the direction I proposed on Slack the other day,

Add a new prop like getRowAction, which returns a CommonMenuItem (the same type that the table actions menu uses) which can have either a to or onClick directive.
a) ... and don't touch rowLinkTo , out of scope tables keep their current behavior
b) ... and replace all uses of rowLinkTo (and handleRowClick) with getRowAction

but that is starting to feel like a lot of churn.

[martha]

This also re-raises this challenge with Bulk Services. To summarize,

• As it exists now (pre-tables updates), Bulk Services uses getColumnDefs which accepts rows (unused in this case) and a loading prop. It uses this loading in an effect for determining the loading state of the Enroll & Assign button.
• However, getColumnDefs is a prop on GenericTableWithData, not GenericTable, and its logic for loading is slightly different:

  hmis-frontend/src/modules/dataFetching/components/GenericTableWithData.tsx

  Line 328 in a028a94

  loading={loading && !data}

• If we try to move the TableRowActions, including the Enroll & Assign button, into the GenericTable, we have to somehow get ahold of GenericTableWithData's version of the loading prop, not just the one GenericTable has (which returns false if there is data, even if it's currently being refetched). Otherwise the effect I linked above does not work, and the button never switches from "Reloading..." to "Assigned"

I would be happy to take a stab at a more satisfactory refactor than I attempted here. (Maybe this is one of those scenarios where "you might not need an effect"?) But it is starting to feel like scope creep... So maybe an acceptable solution for now is to keep this button in a column definition. (in other words, no change from how it's currently implemented.) I think that is still compatible with the other changes we're proposing since this table doesn't have a row click target.

Does all that make sense? What do you think?

[gigxz]

@martha yes I think based on your description, it would be wise to leave the Bed Night workflow table as-is for now, and update it later to add the new menu and address the loading state issues.

[gigxz]

I made a start on implementing this and was happy with how much code cruft we could delete, which makes me feel biased towards the hideMenu prop idea.

Cool I think we should just go with this then. Additional props for customization seem like a fine tradeoff, to make the common case straightforward. If you push up what you have, I can try to wrap it up while you're out. Thank you!

[martha]

Ok, thanks so much @gigxz ! I pushed up what I have. I think it should be almost there. But feel free to make any modifications (rename the new props, etc.) directly on this branch.

I also have been thinking that the diff may be somewhat easier to review once this is merged to the deploy branch, since this PR reverts some of the changes on that branch!