Implement Table Actions pattern and default columns #998

martha · 2024-12-16T17:47:54Z

Description

GH issue: https://github.com/open-path/Green-River/issues/6761

I opened this PR against #989, it should be retargeted to the base branch when that is merged. Both PRs should ideally go out in the same release.

Summary of changes:

Update all in-scope tables (see sheet) to use the new Table Actions pattern and to have the correct columns displayed. Remove any row-click behavior for these tables.
(Attempt to) Standardize common columns and language
- For example, always display an enrollment Entry Date column as an exact date with a relative date tooltip, and the reverse for Last Updated. This involved adding a new component DateWithRelativeTooltip, kind of like the inverse of RelativeDateDisplay.
- Always use "View Client" language (as opposed to "Client Profile")
Consolidate commonly used table row action definitions
Make some changes to the TableRowActions api to enable the Bulk Services use case.
- Enable the primary action to be a React Node
- Add the loading prop passed from the generic table

Other todos:

Co-locate tables that refer to the same model (as opposed to tables that appear on the same dashboard) - I'd like to propose doing this with https://github.com/open-path/Green-River/issues/6848, the diff here is already pretty large.
Run and update spec tests - done Fix spec tests for table actions hmis-warehouse#5012
Expand storybook stories - done in this PR 7a4dd51 where I added a generic story to demonstrate table row actions.

Depends on hmis-warehouse PR: (to do)

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

src/components/elements/table/GenericTable.tsx

martha · 2024-12-16T17:56:16Z

src/modules/admin/components/services/ServiceTypeTable.tsx

@@ -11,11 +12,10 @@ import {
 } from '@/types/gqlTypes';
 import { generateSafePath } from '@/utils/pathEncoding';

-const columns: ColumnDef<ServiceTypeConfigFieldsFragment>[] = [
+const COLUMNS: ColumnDef<ServiceTypeConfigFieldsFragment>[] = [


We had briefly discussed storing all column definitions centrally, in a code-version of the beautiful column spreadsheet. In the end I didn't decide to do that in this PR, because I was worried it would be too much code churn for now. But I'm still somewhat interested in the idea -- you can see in this PR that we have a mix of approaches, some columns defined inline, some defined and exported and used by other components, and some defined in a util file.

I did try to standardize around only using useMemo/useCallback for columns and table actions when they have dependencies in the component props/state, and otherwise defining them in the same file but outside the component, like here. Let me know if you think this is a good approach.

src/modules/hmis/components/RelativeDateDisplay.tsx

martha · 2024-12-16T18:16:27Z

src/modules/projects/components/ProjectAssessments.tsx

+            record.enrollment.client.id,
+            record.enrollment.id
+          ),
+          state: { backToLabel: project.projectName },


Moving this into the table action enables removal of the rowLinkState prop on GenericTable

src/modules/projects/components/tables/ProjectClientEnrollmentsTable.tsx

martha · 2024-12-16T19:02:09Z

src/modules/projects/components/tables/ProjectExternalSubmissionsTable.tsx

+    []
+  );
+
+  const getTableRowActions = useCallback(


Changing this table is out of scope of the ticket (not in the spreadsheet), but it was an especially easy example to update to the new pattern since it already used an Action Button column, so I pulled it in.

martha · 2024-12-16T19:03:20Z

src/modules/search/components/ClientSearch.tsx

-    header: 'Name',
-    key: 'name',
-    render: (client) => <ClientName client={asClient(client)} linkToProfile />,
-  },
  linkedNameNewTab: {


I removed those that weren't used anymore, but there are still a few here (like linkedNameNewTab) that are used in out-of-scope table that I didn't update.

martha · 2024-12-16T19:05:32Z

src/modules/services/serviceColumns.tsx

@@ -6,24 +6,31 @@ import { parseAndFormatDate, serviceDetails } from '@/modules/hmis/hmisUtil';
 import {


Maybe this file should be renamed to util.ts?

martha · 2024-12-16T19:10:15Z

src/modules/assessments/util.tsx

@@ -65,18 +65,6 @@ export const generateAssessmentPath = (
  });
 };

-export const assessmentRowLinkTo = (


I removed this in favor of keeping the assessment link consistent. I think this means that now, any table column that links to an assessment will open it in a household context if applicable. I'm not 100% confident in this change, maybe there was a good reason for the Client assessments page to link to the individual assessment view? I'm torn about whether it seems better to be consistent or maintain the previous behavior, let me know what you think.

This was a design decision so lets review with design before reverting it

I see, thanks. Let's keep the behavior. I think we can still simplify the code by using generateAssessmentPath's individualViewOnly. Existing behavior that this PR replicates per 004ea99:

Household Assessments (qa example -> Household toggle) opens the assessment in household context if applicable

Enrollment Assessments (qa example) opens the assessment in household context if applicable

Client Assessments (qa example) always opens the assessment in individual context

Project Assessments (qa example) always opens the assessment in individual context

gigxz · 2024-12-20T18:46:29Z

src/components/elements/table/TableRowActions.tsx

+      secondaryActions?.map((s) => {
+        return {
+          ...s,
+          ariaLabel: s.ariaLabel || `${s.title}, ${recordName}`,


Do we need to support multiple ways of specifying the aria label? I wonder if we should just make it required on the primary/secondary action definitions, to simplify the logic. (Instead of inferring them based on recordName and title)

Hm yeah I agree that it's not worth the extra code complexity. But I also don't necessarily think we should make ariaLabel required on CommonMenuItem. In general, adding additional aria labels that express something beyond what the button text itself says should be optional.

I'm proposing this middle ground where the primary action's default aria label is set using title and recordName, if not provided in props, while secondary actions can "fend for themselves" in terms of aria labels. I think it's an ok balance. Let me know what you think, though, I'm happy to reconfigure it.

martha · 2025-01-02T18:08:19Z

src/components/elements/table/GenericTable.tsx

@@ -247,7 +243,12 @@ const GenericTable = <T extends { id: string }>({
                width: def.width,
              }}
            >
-              <strong>{def.header}</strong>
+              {def.header ? (


Continuing the thread from here:

{def.key || def.someOtherFieldThatsRequiredIfHeaderIsMissing}

If you agree, I was thinking that based on the comment here,

unique key for element. if not provided, header is used

We probably don't really need to add an extra someOtherFieldThatsRequiredIfHeaderIsMissing, because key should serve exactly that purpose.

Nice -- sounds good, I see that with the union types you achieved that required-ness of header or key. Looks good!

martha · 2025-01-03T15:48:33Z

src/components/elements/RelativeDateDisplay.tsx

@@ -54,7 +56,7 @@ const RelativeDateDisplay = ({
    <Tooltip
      title={
        <Typography component='span' variant='inherit'>
-          {formattedDate}
+          {formattedDate} {tooltipSuffixText}


Moved over from other PR #989 (comment)

martha · 2025-01-03T15:49:51Z

src/modules/services/components/ClientServicesPage.tsx

+                secondaryActionConfigs={[
+                  {
+                    ...getViewEnrollmentMenuItem(row.enrollment, {
+                      id: clientId,


This is the use case for #989 (comment)

gigxz · 2025-01-03T21:42:36Z

src/components/elements/table/tableRowActionUtil.tsx

 ) => {
  return {
    title: 'View Enrollment',
    key: 'enrollment',
-    ariaLabel: `View Enrollment, ${clientBriefName(client)} ${entryExitRange(enrollment)}`,
+    ariaLabel: `View Enrollment, ${client.hasOwnProperty('firstName') ? clientBriefName(client as ClientNameFragment) : ''} ${entryExitRange(enrollment)}`,


Type cast shouldn't be needed / I don't think Pick<ClientNameFragment, 'id'> should be permitted -- I think this is same as other comment on the household PR?

Hm, yes it's the same comment; I moved this change over from the other PR, where it wasn't relevant. But I agree with you that it's a bit messy and could be improved. It fixes the typescript issue arising in ClientServicesPage, where we don't have a client fragment to pass, only client ID - see #998 (comment). The Client Services Page overrides the aria label that calls for the client ID anyhow, so it's purely a typescript issue.

A couple alternative ideas:

Just avoid getViewEnrollmentMenuItem on ClientServicesPage - ok, but leads to some duplication

Use client from dashboard context - seems like a bit of a hassle to keep Typescript happy, but maybe ok?

edit: or the getViewEnrollmentMenuItem function could accept both clientId (required, for url) and client (optional; if provided, then used in aria label)? Or it could require one or the other?

What do you prefer?

Ah I see, thanks. I didn't realize there really was a called that only passed id. I think the approach you added with eee782f is fine but we should not need the cast as ClientNameFragment

martha · 2025-01-06T20:56:06Z

Thanks @gigxz :)

I think this is ready -- what's our typical process for deploy branches? Shall I merge this PR into #989 and then treat that branch as the deploy branch? Should I rename (or merge that branch into a new branch with a clearer name) like deploy/6761-table-actions or similar?

gigxz · 2025-01-06T21:20:56Z

@martha either way is fine (using existing branch name or new branch name)! Lets just make sure that we update the backend test branch greenriver/hmis-warehouse#5012 to have the same branch name, and that shouldn't get merged until the frontend branch is merged.

* Update Project Enrollment tables * fix a11y * More accessibility fixes * Simplify and resolve todos * Add storybook story * Remove errant comment * Add comments * Update api of TableRowActions * Fix issue with fulColSpan * Right-align table row actions Co-authored-by: Gig <[email protected]> * Remove comment * Replace overrideTableBody with more understandable API * Implement PR suggestion of moving table row to column def * Cleanup and add comments * Port over relevant changes from other PR * Refactor to use renderCellContents * Revert out-of-scope change * Add todo * Remove circular dependency * Clean up * Centralize action column base attrs * PR feedback * Fix typescript problems * Implement Table Actions pattern and default columns (#998) * Implement Table Actions pattern and default columns * Add random comment * Add storybook for table actions and fix cde story * Implement PR suggestion of moving table row to column def * add todo * Finish refactor to use column def instead of getTableAction * Fix bulk services loading * Provide a visually hidden header when header isnt provided * Keep existing behavior of individual/household assessment viewing * Fix typescript * Add accessible text for RelativeDateDisplay * implement one possible approach for client ID typescript * Remove unnecessary cast --------- Co-authored-by: Gig Ashton <[email protected]>

Implement Table Actions pattern and default columns

89b582e

martha commented Dec 16, 2024

View reviewed changes

martha requested a review from gigxz December 16, 2024 19:06

martha commented Dec 16, 2024

View reviewed changes

martha added 4 commits December 16, 2024 14:10

Add random comment

50b472b

Merge branch 'release-145' into martha/6761-tables

bb38721

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

e47d030

Add storybook for table actions and fix cde story

7a4dd51

gigxz reviewed Dec 20, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

martha added 6 commits December 30, 2024 19:35

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

401f777

Implement PR suggestion of moving table row to column def

5c4d33a

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

2d2d133

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

8b6a383

add todo

2feec78

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

5026d48

martha mentioned this pull request Jan 2, 2025

Update Project Household Enrollments table with new designs #989

Merged

10 tasks

martha added 5 commits January 2, 2025 09:22

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

39ce9ce

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

f91ea56

Finish refactor to use column def instead of getTableAction

0605c2a

Fix bulk services loading

d5bd5aa

Provide a visually hidden header when header isnt provided

e316ed0

martha commented Jan 2, 2025

View reviewed changes

martha added 4 commits January 2, 2025 13:09

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

7686543

Keep existing behavior of individual/household assessment viewing

004ea99

Merge branch 'martha/6825-hh-member-row' into martha/6761-tables

5bc2ea6

Fix typescript

bdde162

martha commented Jan 3, 2025

View reviewed changes

Add accessible text for RelativeDateDisplay

7339f04

gigxz reviewed Jan 3, 2025

View reviewed changes

implement one possible approach for client ID typescript

eee782f

gigxz approved these changes Jan 6, 2025

View reviewed changes

martha mentioned this pull request Jan 6, 2025

Improve a11y of autofill assessment table #1019

Merged

6 tasks

Remove unnecessary cast

61499cf

martha merged commit 7821056 into martha/6825-hh-member-row Jan 6, 2025
1 check passed

martha deleted the martha/6761-tables branch January 6, 2025 21:55

gigxz mentioned this pull request Jan 9, 2025

Update tables with new Action Column pattern (Deploy Branch) (#989) #1020

Draft

10 tasks

martha mentioned this pull request Jan 28, 2025

Add back rowLinkTo and handleRowClick behaviors #1033

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Table Actions pattern and default columns #998

Implement Table Actions pattern and default columns #998

martha commented Dec 16, 2024 •

edited

Loading

martha Dec 16, 2024

martha Dec 16, 2024

martha Dec 16, 2024

martha Dec 16, 2024

martha Dec 16, 2024

martha Dec 16, 2024

gigxz Dec 23, 2024

martha Jan 2, 2025 •

edited

Loading

gigxz Dec 20, 2024

martha Jan 2, 2025

This comment was marked as outdated.

martha Jan 2, 2025

gigxz Jan 3, 2025

martha Jan 3, 2025

martha Jan 3, 2025

gigxz Jan 3, 2025

martha Jan 3, 2025 •

edited

Loading

gigxz Jan 6, 2025

martha commented Jan 6, 2025

gigxz commented Jan 6, 2025

		@@ -6,24 +6,31 @@ import { parseAndFormatDate, serviceDetails } from '@/modules/hmis/hmisUtil';
		import {

Implement Table Actions pattern and default columns #998

Implement Table Actions pattern and default columns #998

Conversation

martha commented Dec 16, 2024 • edited Loading

Description

Type of change

Checklist before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martha Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martha Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martha commented Jan 6, 2025

gigxz commented Jan 6, 2025

martha commented Dec 16, 2024 •

edited

Loading

martha Jan 2, 2025 •

edited

Loading

martha Jan 3, 2025 •

edited

Loading