Skip to content

Commit

Permalink
✨ table-controls: factor out duplicated code via new usePersistentSta…
Browse files Browse the repository at this point in the history
…te hook + misc cleanup + add initial documentation (`DOCS.md` and JSDoc comments) (#1355)

Before this is merged, the `DOCS.md` file is easier to read by [directly
viewing the markdown-rendered file on my
fork](https://github.com/mturley/tackle2-ui/blob/table-control-hooks-docs/client/src/app/hooks/table-controls/DOCS.md).

This PR got larger than I anticipated... my goal was to write thorough
JSDoc comments and combine the state providers with
`usePersistentState`, but as I documented things I wanted to change I
decided to just go ahead and change them. My sincere apologies for my
self-indulgence and the size of the PR. When you pull on one thread,
there's always another. and another...

* Adds a comprehensive write-up of the rationale, use, and technical
details of the table-control hooks and components in `DOCS.md`.
* Adds JSDoc comments to all files in table-controls for hooks,
components and helpers as well as arguments/options and return values of
each of these.
* Adds `@deprecated` JSDoc comments to the old `useLegacyFilterState`,
`useLegacySortState` and `useLegacyPaginationState` hooks with
explanations.
* Refactors a number of things:
* On `main` currently, each of the `use[Feature]State` hooks is
separated into two almost identical hooks `use[Feature]State` and
`use[Feature]UrlParams` which call `useUrlParams` directly. This is a
lot of duplicated code, including some duplicated business logic. This
PR creates the `usePersistentState` hook which is a drop-in replacement
for `React.useState` that can conditionally persist the state to URL
params, localStorage or sessionStorage. This allows us to combine each
feature's two state hooks into one `use[Feature]State` hook which takes
a `persistTo` argument for configuring the state storage location.
* There are a number of miscellaneous changes to polish the type
definitions and ways types are used across the implementation, including
inheriting certain types from others in a maintainable way. This could
probably use some continued attention after this PR but it is an
improvement.
* The file structure and naming conventions across all features have
been adjusted/renamed/refactored to be consistent.
* The `useTableControlProps` hook was not fully separated along feature
concerns. Some feature-specific code that was in there is moved to a
`use[Feature]PropHelpers` function, and other older `get[Feature]Props`
or `use[Feature]Props` functions are refactored to share this naming
convention.
* The features of the hooks are now opt-in and enabled with boolean
`is[Feature]Enabled` arguments passed to either `useTableControlState`
or `useLocalTableControls`. Using the utility type `DiscriminatedArgs`
and the power of TypeScript discriminated unions, any feature-specific
args for the hooks are only required by TypeScript if that
`is[Feature]Enabled` arg is `true`. This ensures the consumer doesn't
forget anything when enabling and configuring features (for example,
enabling the sort feature and forgetting to specify `sortableColumns`
used to cause sorting to silently fail and now raises a type error).
* I am likely forgetting stuff, this PR went on way too long. Sorry
again about that.

---------

Signed-off-by: Mike Turley <[email protected]>
  • Loading branch information
mturley authored Oct 24, 2023
1 parent abafafa commit ded98a9
Show file tree
Hide file tree
Showing 77 changed files with 2,966 additions and 1,079 deletions.
2 changes: 1 addition & 1 deletion client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"@dnd-kit/sortable": "^7.0.2",
"@hookform/resolvers": "^2.9.11",
"@hot-loader/react-dom": "^17.0.2",
"@migtools/lib-ui": "^9.0.3",
"@migtools/lib-ui": "^10.0.1",
"@patternfly/patternfly": "^5.0.2",
"@patternfly/react-charts": "^7.1.0",
"@patternfly/react-code-editor": "^5.1.0",
Expand Down
3 changes: 2 additions & 1 deletion client/src/app/Constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,9 @@ export enum LocalStorageKey {
}

// URL param prefixes: should be short, must be unique for each table that uses one
export enum TableURLParamKeyPrefix {
export enum TablePersistenceKeyPrefix {
issues = "i",
dependencies = "d",
issuesAffectedApps = "ia",
issuesAffectedFiles = "if",
issuesRemainingIncidents = "ii",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
import React from "react";
import { Td } from "@patternfly/react-table";
import { useTableControlProps } from "@app/hooks/table-controls";
import { ITableControls } from "@app/hooks/table-controls";

export interface ITableRowContentWithControlsProps<
TItem,
TColumnKey extends string,
TSortableColumnKey extends TColumnKey
TSortableColumnKey extends TColumnKey,
TFilterCategoryKey extends string = string,
TPersistenceKeyPrefix extends string = string,
> {
expandableVariant?: "single" | "compound" | null;
isSelectable?: boolean;
propHelpers: ReturnType<
typeof useTableControlProps<TItem, TColumnKey, TSortableColumnKey>
isExpansionEnabled?: boolean;
expandableVariant?: "single" | "compound";
isSelectionEnabled?: boolean;
propHelpers: ITableControls<
TItem,
TColumnKey,
TSortableColumnKey,
TFilterCategoryKey,
TPersistenceKeyPrefix
>["propHelpers"];
item: TItem;
rowIndex: number;
Expand All @@ -20,22 +27,23 @@ export interface ITableRowContentWithControlsProps<
export const TableRowContentWithControls = <
TItem,
TColumnKey extends string,
TSortableColumnKey extends TColumnKey
TSortableColumnKey extends TColumnKey,
>({
expandableVariant = null,
isSelectable = false,
propHelpers: { getSingleExpandTdProps, getSelectCheckboxTdProps },
isExpansionEnabled = false,
expandableVariant,
isSelectionEnabled = false,
propHelpers: { getSingleExpandButtonTdProps, getSelectCheckboxTdProps },
item,
rowIndex,
children,
}: React.PropsWithChildren<
ITableRowContentWithControlsProps<TItem, TColumnKey, TSortableColumnKey>
>) => (
<>
{expandableVariant === "single" ? (
<Td {...getSingleExpandTdProps({ item, rowIndex })} />
{isExpansionEnabled && expandableVariant === "single" ? (
<Td {...getSingleExpandButtonTdProps({ item, rowIndex })} />
) : null}
{isSelectable ? (
{isSelectionEnabled ? (
<Td {...getSelectCheckboxTdProps({ item, rowIndex })} />
) : null}
{children}
Expand Down
6 changes: 2 additions & 4 deletions client/src/app/components/answer-table/answer-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,12 @@ const AnswerTable: React.FC<IAnswerTableProps> = ({
choice: "Answer choice",
weight: "Weight",
},
hasActionsColumn: false,
hasPagination: false,
variant: "compact",
});
const {
currentPageItems,
numRenderedColumns,
propHelpers: { tableProps, getThProps, getTdProps },
propHelpers: { tableProps, getThProps, getTrProps, getTdProps },
} = tableControls;

const getIconByRisk = (risk: string): React.ReactElement => {
Expand Down Expand Up @@ -118,7 +116,7 @@ const AnswerTable: React.FC<IAnswerTableProps> = ({
{currentPageItems?.map((answer, rowIndex) => {
return (
<>
<Tr key={answer.text}>
<Tr key={answer.text} {...getTrProps({ item: answer })}>
<TableRowContentWithControls
{...tableControls}
item={answer}
Expand Down
6 changes: 4 additions & 2 deletions client/src/app/components/questions-table/questions-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const QuestionsTable: React.FC<{
formulation: "Name",
section: "Section",
},
isExpansionEnabled: true,
expandableVariant: "single",
forceNumRenderedColumns: isAllQuestionsTab ? 3 : 2, // columns+1 for expand control
});
Expand All @@ -54,6 +55,7 @@ const QuestionsTable: React.FC<{
propHelpers: {
tableProps,
getThProps,
getTrProps,
getTdProps,
getExpandedContentTdProps,
},
Expand All @@ -63,7 +65,7 @@ const QuestionsTable: React.FC<{
const { t } = useTranslation();

return (
<Table {...tableProps} aria-label="Questions table" isExpandable>
<Table {...tableProps} aria-label="Questions table">
<Thead>
<Tr>
<TableHeaderContentWithControls {...tableControls}>
Expand Down Expand Up @@ -98,7 +100,7 @@ const QuestionsTable: React.FC<{
)?.name || "";
return (
<>
<Tr key={question.text}>
<Tr key={question.text} {...getTrProps({ item: question })}>
<TableRowContentWithControls
{...tableControls}
item={question}
Expand Down
Loading

0 comments on commit ded98a9

Please sign in to comment.