Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation (DOCS.md and JSDoc comments) #1355

Merged
merged 102 commits into from
Oct 24, 2023

Conversation

mturley
Copy link
Collaborator

@mturley mturley commented Sep 13, 2023

Before this is merged, the DOCS.md file is easier to read by directly viewing the markdown-rendered file on my fork.

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 useState or useUrlParams directly. This is a lot of duplicated code, including some duplicated business logic. This PR creates the usePersistentState hook which is a 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]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley force-pushed the table-control-hooks-docs branch 2 times, most recently from 3f0af01 to b1e43db Compare September 13, 2023 21:33
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Attention: 269 lines in your changes are missing coverage. Please review.

Comparison is base (abafafa) 40.72% compared to head (9e0146f) 40.31%.

❗ Current head 9e0146f differs from pull request most recent head 06546cb. Consider uploading reports for the commit 06546cb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1355      +/-   ##
==========================================
- Coverage   40.72%   40.31%   -0.41%     
==========================================
  Files         139      143       +4     
  Lines        4469     4524      +55     
  Branches     1069     1108      +39     
==========================================
+ Hits         1820     1824       +4     
- Misses       2554     2603      +49     
- Partials       95       97       +2     
Flag Coverage Δ
client 40.31% <19.70%> (-0.41%) ⬇️
server ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
client/src/app/Constants.ts 100.00% <100.00%> (ø)
.../src/app/hooks/table-controls/active-item/index.ts 100.00% <100.00%> (ø)
...nt/src/app/hooks/table-controls/expansion/index.ts 100.00% <100.00%> (ø)
...le-controls/filtering/getFilterHubRequestParams.ts 10.93% <ø> (ø)
...nt/src/app/hooks/table-controls/filtering/index.ts 100.00% <100.00%> (ø)
...rc/app/hooks/table-controls/getHubRequestParams.ts 41.66% <ø> (ø)
client/src/app/hooks/table-controls/index.ts 100.00% <100.00%> (ø)
...ntrols/pagination/getPaginationHubRequestParams.ts 16.66% <ø> (ø)
...t/src/app/hooks/table-controls/pagination/index.ts 100.00% <100.00%> (ø)
.../table-controls/sorting/getSortHubRequestParams.ts 18.18% <ø> (ø)
... and 28 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley force-pushed the table-control-hooks-docs branch 2 times, most recently from 8d1e616 to 66a5fe8 Compare September 14, 2023 15:06
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley changed the title 📖 [WIP] Add documentation for table-controls hooks, helpers and components 📖 [WIP] Initial documentation for table-controls hooks, helpers and components Sep 14, 2023
@mturley mturley force-pushed the table-control-hooks-docs branch 2 times, most recently from 7eff52e to 2c56571 Compare September 22, 2023 19:54
@mturley mturley force-pushed the table-control-hooks-docs branch 3 times, most recently from 3816352 to 22f783e Compare September 22, 2023 21:12
mturley added a commit that referenced this pull request Sep 26, 2023
…able to use it (#1392)

When using the table-control hooks for a server-paginated table, you can
use either `useTableControlState` or `useTableControlUrlParams` to
provide state, then you take the returned object from that and use its
properties to fetch API data, then pass the object with additional
API-dependent properties to `useTableControlProps`.

For a client-paginated table, there is no need to split this into two
steps because the API request is not dependent on the table state
(there's no need to take pagination/sort/filter state and include it in
the API request), so the shorthand `useLocalTableControls` hook exists
which is simply `useTableControlProps(useLocalTableControlState(args))`
(`useLocalTableControlState` is similar to `useTableControlState` but
performs client-side logic with the `getLocal[Feature]DerivedState`
helpers in between calling each feature hook). However,
`useLocalTableControls` only allows the use of React state as the source
of truth rather than URL params. This PR adds an equivalent
`useLocalTableControlsWithUrlParams` shorthand hook that is a drop-in
replacement for `useLocalTableControls` which uses URL params instead of
React state.

The only additional argument available when switching to this new hook
is `urlParamKeyPrefix`, which is optional and is used to distinguish the
params for multiple tables on the same page. Even though the archetypes
table is the only table on its page, I used it to be consistent in case
we do add additional tables in modals or drawers, etc. like we did for
Issues / Affected apps.

With the archetypes table now using URL params for its filter state, we
should now easily be able to navigate to the archetypes table filtered
by tags (as discussed with @ibolton336) by using an approach similar to
`getAffectedAppsUrl` on the issues page ([usage
here](https://github.com/konveyor/tackle2-ui/blob/main/client/src/app/pages/issues/affected-apps-link.tsx#L22C1-L28),
[implementation
here](https://github.com/konveyor/tackle2-ui/blob/main/client/src/app/pages/issues/helpers.ts#L100)),
which uses the `trimAndStringifyUrlParams` and
`serializeFilterUrlParams` helpers. This implementation might look
something like:

```ts
export const getArchetypesUrlFilteredByTags = (tags: string[]) => {
  const baseUrl = Paths.archetypes;
  return `${baseUrl}?${trimAndStringifyUrlParams({
    newPrefixedSerializedParams: {
      [`${TableURLParamKeyPrefix.archetypes}:filters`]: serializeFilterUrlParams({ tags }).filters,
    },
  })}`;
};
```

Note (see the code comment in this diff): Because of the way we
implemented `useUrlParams`, there is no way to conditionally use URL
params or React state within the same hook based on an argument. This is
why all the separate `use[Feature]State` and `use[Feature]UrlParams`
hooks exist, and it also results in a moderate amount of duplication of
code in this PR because of the logic required to feed the values
returned from these feature hooks into each other. In the future we
should refactor `useUrlParams` into something like `useStateOrUrlParams`
to eliminate this issue and the duplication it causes (I also mentioned
this in the initial docs in #1355). That will come in a future PR.

Signed-off-by: Mike Turley <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
@mturley mturley changed the title ✨ [WIP] table-controls: factor out duplicated code via usePersistentState + misc cleanup + add initial documentation ✨ [WIP] table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation Oct 18, 2023
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley changed the title ✨ [WIP] table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation ✨ [WIP] table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation (DOCS.md and JSDoc comments) Oct 23, 2023
@mturley mturley changed the title ✨ [WIP] table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation (DOCS.md and JSDoc comments) ✨ [WIP] table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation (DOCS.md and JSDoc comments) Oct 23, 2023
@mturley mturley changed the title ✨ [WIP] table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation (DOCS.md and JSDoc comments) ✨ table-controls: factor out duplicated code via new usePersistentState hook + misc cleanup + add initial documentation (DOCS.md and JSDoc comments) Oct 23, 2023
@mturley mturley marked this pull request as ready for review October 23, 2023 19:55
Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great @mturley. Thanks for all of the hard work on this. The docs are going to be a lifesaver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants