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

✨ Add useLocalTableControlsWithUrlParams hook and update archetypes table to use it #1392

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

mturley
Copy link
Collaborator

@mturley mturley commented Sep 22, 2023

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, implementation here), which uses the trimAndStringifyUrlParams and serializeFilterUrlParams helpers. This implementation might look something like:

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.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

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

Comparison is base (15ec166) 41.37% compared to head (bacd72d) 41.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1392      +/-   ##
==========================================
- Coverage   41.37%   41.22%   -0.15%     
==========================================
  Files         138      138              
  Lines        4326     4349      +23     
  Branches     1037     1043       +6     
==========================================
+ Hits         1790     1793       +3     
- Misses       2448     2468      +20     
  Partials       88       88              
Flag Coverage Δ
client 41.22% <28.57%> (-0.15%) ⬇️
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%> (ø)
...p/hooks/table-controls/useTableControlUrlParams.ts 46.15% <ø> (ø)
.../app/hooks/table-controls/useLocalTableControls.ts 66.66% <66.66%> (-8.34%) ⬇️
.../hooks/table-controls/useLocalTableControlState.ts 17.39% <20.83%> (-9.54%) ⬇️

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

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. I just have a suggestion to help with the code duplication....

isItemSelectable,
});

const sortState = useSortUrlParams({ ...args, sortableColumns, initialSort });
Copy link
Member

Choose a reason for hiding this comment

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

You could use a bit of trickery to get this as a single call .....

  const isUrlParamBased = !!urlParamKeyPrefix;

  const sortState = (isUrlParamBased ? useSortUrlParams : useSortState)({
    ...args, 
    sortableColumns,
    initialSort,
  });

Copy link
Collaborator Author

@mturley mturley Sep 26, 2023

Choose a reason for hiding this comment

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

I considered this, but the eslint rules-of-hooks rule doesn't like it I think (if you re-render with a different value for isUrlParamBased it would switch up which hooks are called and screw up the render, not that we'd do that). If we added an eslint-ignore there I think it would be too easy to accidentally break it in a future change.

What I'd like to do eventually (soon perhaps) is refactor useUrlParams (which is used inside useSortUrlParams and each of the other use[Feature]UrlParams hooks) into a useStateOrUrlParams so that it itself has a isUrlParamBased condition, and has its own useState internally that it either uses or ignores in favor of URL params depending on config. Then we could get rid of all the use[Feature]UrlParams hooks and instead have the URL params be an option in use[Feature]State, all the way up to useTableControlState.

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.

4 participants