Update Project Household Enrollments table with new designs #989

martha · 2024-12-05T14:43:34Z

Description

Should be merged into the same release as #998

GH issue: https://github.com/open-path/Green-River/issues/6825
Depends on hmis-warehouse PR: greenriver/hmis-warehouse#5012 This is not a functional dependency, but is needed for spec tests to pass.

This PR

Updates the ProjectHouseholdsTable component to render each household member in their own row
Defaults to the Clients tab instead of Households tab
Updates the columns rendered in both the ProjectHouseholdsTable and the ProjectClientEnrollmentsTable
Implements a new TableRowActions that will be more widely adopted with https://github.com/open-path/Green-River/issues/6761
Adds a basic storybook story

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/CommonMenuButton.tsx

martha · 2024-12-05T15:58:05Z

src/modules/hmis/hmisUtil.ts

+
+// TODO @MARTHA discuss - it's probably a bit silly to redefine ranks here,
+// see https://github.com/greenriver/hmis-warehouse/blob/1a170c0e869fd3d69880532f86baa104b800d9ed/drivers/hmis/app/graphql/types/hmis_schema/enums/hud.rb#L723-L733
+const hohPriorityMapping: Record<RelationshipToHoH, number> = {


This is for sorting the household members by their relationship to HoH. This definition of the ordering is redundant with the backend enum definition, so I wanted to discuss whether there's a better way to pass that info to the frontend.

I don't think it's redundant with the backend. The enum lists out the values which are the HUD numeric values (Eg child = 2) whereas here you're specifying a custom order for displaying household members. It seems correct to me that this would live on the frontend.

martha · 2024-12-05T16:12:32Z

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

+        return (
+          <>
+            <TableBody
+              // TODO @martha - discuss these TableBodies to achieve the `rowgroup` role, is it worth it?


Using the role='rowgroup' turned out to be a bit complicated because of MUI expectations around usage of the TableBody, TableRow and TableCell components. We can't directly have renderRow return a <div role='rowgroup'> which contains all the TableRows within it, because MUI expects that TableRow is a direct child of TableBody. (Here's a screenshot of what happens:)

Rendering each household as its own tbody is the best solution I was able to come up with, but I'm not a huge fan -- both because of the messy markup and because of bloating the already complicated GenericTable api. Let me know if you can think of any better solutions. Or, we could skip using rowgroup. I think the accessibility is still better than before. We could, for example, give the row header cell additional accessible text specifying the household (HoH name or household ID) that would be read when navigating the screen with a screenreader.

using several tbodys seems like a fine approach. from mdn,

It's permitted to use more than one per table as long as they are all consecutive. This allows to divide the rows ( elements) in large tables into sections, each of which may be separately formatted if so desired.

That seems like basically what we are doing.

overrideTableBody prop maybe isn't the clearest as to what it's doing, since it's really omitting the tbody tag entirely, not overriding it. Maybe another idea is passing TableBodyComponent={React.Fragment} where the default is TableBodyComponent = TableBody, then the passed component can be used directly? Either seems OK.

Ok, thank you for validating the overall approach! I like your api suggestion, took a stab at implementing that here: 3586e74

martha · 2024-12-05T16:16:55Z

src/modules/projects/components/tables/ProjectHouseholdsTable.stories.tsx

@@ -0,0 +1,26 @@
+import { Card } from '@mui/material';


We could add more stories here to test different cases, but I was thinking to leave it as is and create more fleshed-out stories for GenericTable with 6761, or later. Let me know what you think.

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

src/components/elements/table/TableRowActions.tsx

gigxz · 2024-12-20T18:26:03Z

src/components/elements/table/GenericTable.tsx

+  // This should only be used by tables that take over rendering using renderRow and render a `tbody` within their custom render fn
+  overrideTableBody?: boolean;
+  injectBelowRows?: ReactNode; // component to inject below all rendered rows, above footer
+  getTableRowActions?: (record: T) => TableRowActionsType;


I'm wondering about the decision to make this a prop as opposed to just another column definition, can you say more about that? It does add more complexity to the already-complex GenericTable, so just wanting to think that through.

By a column definition, I mean:

const columns = [ ...othercols, { key: 'actions', render: (enrollment) => ( <TableRowActions record={enrollment} primaryAction={viewEnrollmentMenuItemConfig(enrollment} secondaryActions={[viewClientMenuItemConfig(enrollment)]} /> ), }, ] ... <GenericTableWithData columns={columns} />

Looking at https://github.com/greenriver/hmis-frontend/pull/998/files, I wonder if this would also give us some more flexibility for use-cases when the action button is more complex. I haven't quite wrapped my head around everything yet, but would something like this simplify the changes in the other PR ?

Definitely an early thought, I'm sure there are things I'm missing about the implementation.

const columns = [ ...othercols, { key: 'actions', render: (enrollment) => ( <TableRowActions record={enrollment} // use this prop if the primary action is a simple navigation primaryActionConfig={viewEnrollmentMenuItemConfig(enrollment} // use this prop if the primary action is more complicated, or if needs some special treatment renderPrimaryAction={<AssignServiceButton .... />} // maybe secondary actions are always expected to be navigation secondaryActionsConfig={[viewClientMenuItemConfig(enrollment)]} /> ), }, ] ... <GenericTableWithData columns={columns} />

I didn't think thoroughly about it, but it was based on the idea that, since this is a new pattern implemented consistently across many tables in the app, we want the common table component to guide usage and force consistency.

Thinking beyond table row actions, the logical conclusion of that train of thought is that all table controls, like search, filter, toggle, add-record button, etc. live as props on the GenericTable. That doesn't seem desirable - it certainly would lead to a bloated GenericTable. But I guess forcing consistency is the one upside. Currently we rely on GenericTable callers to implement search bars and add-one buttons consistently every time, and there are places in the app where we aren't consistent. (For example, search and toggle on enrollments table vs. search on projects/orgs.)

Anyway, I guess those considerations seem out of scope... I don't feel strongly about it and can take a stab at implementing your suggestion.

quick example of the consistency issue I'm mentioning, as actually applied to table row actions: https://github.com/greenriver/hmis-frontend/blob/martha/6825-hh-member-row/src/components/elements/table/GenericTable.tsx#L259-L261 By moving this out of GenericTable and into a regular column definition, we leave the column header (or lack thereof / accessibility thereof) up to the caller

I've taken a shot at the refactor, I'd like to look at it together tomorrow: https://github.com/greenriver/hmis-frontend/compare/martha/6761-tables...martha/6761-tables-refactor?expand=1

we leave the column header (or lack thereof / accessibility thereof) up to the caller

This seems like something that should be addressed globally for all columns. The ColumnDef has the header attribute as optional, so any column could be missing a header and become inaccessible.

Looking at how we implement the Header cells:

hmis-frontend/src/components/elements/table/GenericTable.tsx

Lines 225 to 236 in 91e938c

{columns.map((def, i) => (

<HeaderCell

key={key(def) || i}

sx={{

...(headerCellSx ? headerCellSx(def) : undefined),

textAlign: def.textAlign,

width: def.width,

}}

>

<strong>{def.header}</strong>

</HeaderCell>

))}

it could be modified so that it sets a hidden header if no visible header is provided:

<HeaderCell> {def.header ? ( <strong>{def.header}</strong> ) : ( <Box sx={visuallyHidden}> {def.key || def.someOtherFieldThatsRequiredIfHeaderIsMissing} </Box> )} </HeaderCell>

That might address the concern more widely -- we could add additional attributes to ColumnDef if needed, to ensure that columns always have accessible headers.

good idea, I've left a todo here that I'd like to address in the other PR

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

Co-authored-by: Gig <[email protected]>

martha · 2024-12-31T23:02:00Z

src/modules/client/components/ClientEnrollmentCard.tsx

          render: 'projectName',
        },
-        ENROLLMENT_PERIOD_COL,
+        {


Pulled this out to here because this is now the only place where it is being used.

martha · 2024-12-31T23:06:42Z

src/modules/hmis/components/DateWithRelativeTooltip.tsx

@@ -0,0 +1,59 @@
+import {


I pulled this change into here, from the other PR, since it's relevant to the Enrollment tables updated in this PR.

Nice. I never noticed that this was part of the design. It does look like we are missing part of the tooltip contents:

Most accessible text for cells with hover-over interactions should include both cell and tooltip text (2/5/25, 6 days ago’)

I think this file and RelativeDateDisplay should go in src/components/elements with the rest of the relative date components. (And hopefully we can consolidate later).
The hmis module has general hmis-domain-specific components, like HmisField,HohIndicator, etc. This one isn't domain specific so I think it goes in src.

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

martha · 2025-01-02T14:12:00Z

src/components/elements/table/GenericTable.tsx

+  // This should only be used by tables that take over rendering using renderRow and render a `tbody` within their custom render fn
+  overrideTableBody?: boolean;
+  injectBelowRows?: ReactNode; // component to inject below all rendered rows, above footer
+  getTableRowActions?: (record: T) => TableRowActionsType;


good idea, I've left a todo here that I'd like to address in the other PR

gigxz · 2025-01-02T16:52:54Z

Is this ready for re-review @martha? And can you re-target to 147? thanks!

martha · 2025-01-02T16:57:53Z

@gigxz, yes, I think it's ready for re-review. Thank you!

gigxz

looks good!

src/components/elements/table/GenericTable.tsx

src/components/elements/table/tableRowActionUtil.tsx

gigxz · 2025-01-02T22:08:56Z

src/modules/hmis/components/DateWithRelativeTooltip.tsx

@@ -0,0 +1,59 @@
+import {


Nice. I never noticed that this was part of the design. It does look like we are missing part of the tooltip contents:

Most accessible text for cells with hover-over interactions should include both cell and tooltip text (2/5/25, 6 days ago’)

gigxz · 2025-01-02T22:09:38Z

src/modules/hmis/components/DateWithRelativeTooltip.tsx

@@ -0,0 +1,59 @@
+import {


I think this file and RelativeDateDisplay should go in src/components/elements with the rest of the relative date components. (And hopefully we can consolidate later).
The hmis module has general hmis-domain-specific components, like HmisField,HohIndicator, etc. This one isn't domain specific so I think it goes in src.

src/modules/hmis/components/RelativeDateDisplay.tsx

gigxz · 2025-01-02T22:14:50Z

src/modules/hmis/hmisUtil.ts

+
+// TODO @MARTHA discuss - it's probably a bit silly to redefine ranks here,
+// see https://github.com/greenriver/hmis-warehouse/blob/1a170c0e869fd3d69880532f86baa104b800d9ed/drivers/hmis/app/graphql/types/hmis_schema/enums/hud.rb#L723-L733
+const hohPriorityMapping: Record<RelationshipToHoH, number> = {


I don't think it's redundant with the backend. The enum lists out the values which are the HUD numeric values (Eg child = 2) whereas here you're specifying a custom order for displaying household members. It seems correct to me that this would live on the frontend.

src/modules/hmis/hmisUtil.ts

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

gigxz

📈 looks good! thanks for your work and iteration!!

gigxz · 2025-01-03T21:23:39Z

@martha merge conflicts might be a pain with this and #998? If you want to hold off on squashing this into main until it contains #998, that would be fine w/ me. (I think that would avoid the conflicts)

Another option is a deploy branch, so this can get plenty of review on QA with design (without being tied to regular releases) - lets revisit Monday!

* 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

…1020)

martha added 5 commits December 4, 2024 17:56

Update Project Enrollment tables

05d57ed

fix a11y

3ca2844

More accessibility fixes

d4f8372

Simplify and resolve todos

3fac8a7

Add storybook story

9aad6be

martha mentioned this pull request Dec 5, 2024

Fix spec tests with new Household Enrollments table design greenriver/hmis-warehouse#4983

Closed

3 tasks

martha added 2 commits December 5, 2024 10:49

Remove errant comment

32c4da4

Add comments

6f0c3ad

martha marked this pull request as ready for review December 5, 2024 16:15

martha commented Dec 5, 2024

View reviewed changes

martha requested a review from gigxz December 5, 2024 16:22

martha commented Dec 10, 2024

View reviewed changes

src/components/elements/table/TableRowActions.tsx Outdated Show resolved Hide resolved

martha added 2 commits December 11, 2024 07:50

Update api of TableRowActions

8368350

Fix issue with fulColSpan

d744d24

martha changed the base branch from release-143 to release-144 December 11, 2024 20:57

Merge branch 'release-144' into martha/6825-hh-member-row

8b51bd4

martha mentioned this pull request Dec 16, 2024

Implement Table Actions pattern and default columns #998

Merged

13 tasks

Merge branch 'release-145' into martha/6825-hh-member-row

3a3cf64

martha changed the base branch from release-144 to release-146 December 19, 2024 14:06

Merge branch 'release-146' into martha/6825-hh-member-row

4c6d640

gigxz reviewed Dec 20, 2024

View reviewed changes

src/components/elements/table/TableRowActions.tsx Outdated Show resolved Hide resolved

gigxz reviewed Dec 20, 2024

View reviewed changes

src/modules/projects/components/tables/ProjectHouseholdsTable.tsx Outdated Show resolved Hide resolved

martha and others added 7 commits December 30, 2024 18:31

Merge branch 'release-146' into martha/6825-hh-member-row

7211ab2

Right-align table row actions

e26a57e

Co-authored-by: Gig <[email protected]>

Remove comment

ae6b49a

Replace overrideTableBody with more understandable API

3586e74

Implement PR suggestion of moving table row to column def

a3b888a

Cleanup and add comments

197e12b

Port over relevant changes from other PR

f96f87d

martha added 4 commits December 31, 2024 17:58

Refactor to use renderCellContents

359c9b8

Revert out-of-scope change

2da7754

Add todo

39eab24

Remove circular dependency

e919970

martha commented Jan 2, 2025

View reviewed changes

martha added 2 commits January 2, 2025 09:19

Clean up

0c67ce0

Centralize action column base attrs

8613a8d

martha changed the base branch from release-146 to release-147 January 2, 2025 16:56

Merge branch 'release-147' into martha/6825-hh-member-row

0380e2b

gigxz reviewed Jan 2, 2025

View reviewed changes

martha added 2 commits January 3, 2025 10:03

PR feedback

9ffaa63

Fix typescript problems

cee0dbf

gigxz approved these changes Jan 3, 2025

View reviewed changes

martha changed the base branch from release-147 to deploy/6761-table-actions January 6, 2025 21:55

martha merged commit 4bb7e11 into deploy/6761-table-actions Jan 6, 2025
3 checks passed

martha deleted the martha/6825-hh-member-row branch January 6, 2025 21:59

gigxz mentioned this pull request Jan 9, 2025

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

Merged

10 tasks

gigxz pushed a commit that referenced this pull request Feb 6, 2025

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

0cc2a9e

…1020)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Project Household Enrollments table with new designs #989

Update Project Household Enrollments table with new designs #989

martha commented Dec 5, 2024 •

edited

Loading

martha Dec 5, 2024

gigxz Jan 2, 2025

martha Dec 5, 2024

gigxz Dec 20, 2024

martha Dec 31, 2024

martha Dec 5, 2024

gigxz Dec 20, 2024 •

edited

Loading

gigxz Dec 20, 2024

martha Dec 31, 2024

martha Dec 31, 2024

martha Dec 31, 2024

gigxz Dec 31, 2024

martha Jan 2, 2025 •

edited

Loading

martha Dec 31, 2024

martha Dec 31, 2024

gigxz Jan 2, 2025

gigxz Jan 2, 2025

martha Jan 2, 2025 •

edited

Loading

gigxz commented Jan 2, 2025

martha commented Jan 2, 2025

gigxz left a comment

gigxz Jan 2, 2025

gigxz Jan 2, 2025

gigxz Jan 2, 2025

gigxz left a comment

gigxz commented Jan 3, 2025 •

edited

Loading

	{columns.map((def, i) => (
	<HeaderCell
	key={key(def) \|\| i}
	sx={{
	...(headerCellSx ? headerCellSx(def) : undefined),
	textAlign: def.textAlign,
	width: def.width,
	}}
	>
	<strong>{def.header}</strong>
	</HeaderCell>
	))}

Update Project Household Enrollments table with new designs #989

Update Project Household Enrollments table with new designs #989

Conversation

martha commented Dec 5, 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

gigxz Dec 20, 2024 • edited Loading

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

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

gigxz commented Jan 2, 2025

martha commented Jan 2, 2025

gigxz left a 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

gigxz left a comment

Choose a reason for hiding this comment

gigxz commented Jan 3, 2025 • edited Loading

martha commented Dec 5, 2024 •

edited

Loading

gigxz Dec 20, 2024 •

edited

Loading

martha Jan 2, 2025 •

edited

Loading

martha Jan 2, 2025 •

edited

Loading

gigxz commented Jan 3, 2025 •

edited

Loading