feat(autgrid): Add custom filtering option to columns #1911

krissvaa · 2023-12-20T11:08:36Z

Provide headerFilterRender for users as a new property in columnOptions
Split headerRenderer and headerFilterRenderer to solve consistency issue with headerRenderer being responsible for two places.
Provide setPropertyFilter function to headerFilterRenderer as a property in columnOptions
Add identifier to andFilter and orFilter (key) for controlling (delete/replace/add) filter list
Change setPropertyFilter to accept any Filter, not only StringPropertyFilter
Enable adding headerFilterRenderer to custom columns
Enable setting headerRenderer from columnOptions as the primary source (overwriting default)
Provide types to users: ColumnOptions, HeaderFilterRendererProps

codecov · 2023-12-20T11:14:37Z

Codecov Report

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

Comparison is base (bb4aced) 95.27% compared to head (7be51df) 95.24%.

Files	Patch %	Lines
packages/ts/react-crud/src/autogrid.tsx	92.85%	0 Missing and 2 partials ⚠️
packages/ts/react-crud/src/autogrid-columns.tsx	66.66%	0 Missing and 1 partial ⚠️
packages/ts/react-crud/src/util.tsx	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1911      +/-   ##
==========================================
- Coverage   95.27%   95.24%   -0.04%     
==========================================
  Files          52       52              
  Lines        3449     3469      +20     
  Branches      503      514      +11     
==========================================
+ Hits         3286     3304      +18     
+ Misses        136      134       -2     
- Partials       27       31       +4

Flag	Coverage Δ
unittests	`95.24% <91.30%> (-0.04%)`	⬇️

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

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

sissbruecker · 2023-12-21T08:01:04Z

packages/java/endpoint/src/main/java/dev/hilla/crud/filter/AndFilter.java

@@ -11,6 +11,8 @@
 public class AndFilter extends Filter {
    private List<Filter> children;

+    private String key;


It seems we only need this internally in order to be able to replace the correct filter instance in the root filter of the AutoGrid. If that is the case then adding this as a required property to the public API isn't a good solution (also a breaking change). I think it will also be confusing to developers what this key is and why they have to manage it.

I think we could manage this key internally / automatically. We could add a key parameter to AutoGrid's setPropertyFilter. The key could be the column's key, or an incremental number stored in a ref. When exposing the setPropertyFilter to the header filters, we do not pass the original function, but a wrapper function that automatically sets the key:

export function InternalHeaderFilterRenderer({ original }: HeaderRendererProps): JSX.Element | null { const { setPropertyFilter: setPropertyFilterOriginal, headerFilterRenderer: HeaderFilterRenderer, filterKey, // Would be determined somehow when rendering the context } = useContext(ColumnContext)!; function setPropertyFilter(filter: FilterUnion) { setPropertyFilterOriginal(filterKey, filter); } return <HeaderFilterRenderer original={original} setPropertyFilter={setPropertyFilter} />; }

Agree and I implemented your suggested changes.
Only code will use the key property for custom columns and property.name for model-type columns as identifiers for filter management.

sissbruecker · 2023-12-21T08:10:02Z

packages/ts/react-crud/src/autogrid-columns.tsx

@@ -105,15 +108,31 @@ export function getColumnOptions(
  customColumnOptions: ColumnOptions | undefined,
 ): ColumnOptions {
  const typeColumnOptions = getTypeColumnOptions(propertyInfo);
-  const finalHeaderRenderer =
-    customColumnOptions?.filterable === false ? NoHeaderFilter : typeColumnOptions.headerRenderer;
+  const HeaderFilterRenderer =


Nit: IMO writing this in upper-case creates visual noise for no real benefit. It could just be headerFilterRenderer and then be cleanly applied in the object below with a shorthand assignment.

sissbruecker · 2023-12-21T08:13:05Z

packages/ts/react-crud/src/autogrid-columns.tsx

  }
  return columnOptions;
 }
+
+export function InternalHeaderFilterRenderer({ original }: HeaderRendererProps): JSX.Element | null {


I think these wrappers would a better fit for header-filter.tsx.

Since they effectively do the same, maybe combine them into one:

export function InternalHeaderFilterRenderer({ original }: HeaderRendererProps): JSX.Element | null { const { setPropertyFilter, headerFilterRenderer: HeaderFilterRenderer } = (useContext(ColumnContext) ?? useContext(CustomColumnContext))!; return <HeaderFilterRenderer original={original} setPropertyFilter={setPropertyFilter} />; }

The name could also be improved to clarify its intent.

sissbruecker · 2023-12-21T08:16:26Z

packages/ts/react-crud/src/autogrid-columns.tsx

-  if (!columnOptions.headerRenderer) {
-    console.error(`No header renderer defined for column ${propertyInfo.name}`);
+  if (!columnOptions.headerFilterRenderer) {
+    console.error(`No filter renderer defined for column ${propertyInfo.name}`);


Does this serve a purpose? Should we just use NoHeaderFilter as a fallback?

sissbruecker · 2023-12-21T08:16:38Z

packages/ts/react-crud/src/autogrid-columns.tsx

+  if (HeaderFilterRenderer) {
+    return <HeaderFilterRenderer original={original} setPropertyFilter={setPropertyFilter} />;
+  }
+  return null;


Can solve the missing code coverage here by making headerFilterRenderer required, and using NoHeaderFilter if no custom renderer is specified.

sissbruecker · 2023-12-21T08:19:17Z

packages/ts/react-crud/src/autogrid.tsx

+  const { key } = column;
+  const { header, headerRenderer: HeaderRenderer } = column.props;
+  const customOptions = options.columnOptions?.[key!];
+  const { header: customHeader, headerRenderer: CustomHeaderRenderer, headerFilterRenderer } = customOptions ?? {};


Nit: Likewise, the uppercase here doesn't provide any benefit.

sissbruecker · 2023-12-21T08:21:41Z

packages/ts/react-crud/src/autogrid.tsx

@@ -186,7 +219,7 @@ function addCustomColumns(columns: JSX.Element[], options: ColumnConfigurationOp

 function useColumns(
  properties: PropertyInfo[],
-  setPropertyFilter: (propertyFilter: PropertyStringFilter) => void,
+  setPropertyFilter: (filter: PropertyStringFilter) => void,


Shouldn't this use FilterUnion?

sissbruecker · 2023-12-21T08:24:23Z

packages/ts/react-crud/src/autogrid.tsx

+    }
+    return null;
+  }
+  const isEmpty = (filter: FilterUnion): boolean => {


Looks like this could use more test coverage. Maybe consider extracting this into a utility and testing it separately.

sissbruecker · 2023-12-21T08:29:04Z

packages/ts/react-crud/src/header-filter.tsx

+  /**
+   * Custom renderer for the title/sorter in the header.
+   */
+  headerRenderer?: ComponentType<HeaderRendererProps>;


Doesn't look like this is needed, ColumnOptions already has headerRenderer from GridColumnProps.

sissbruecker · 2023-12-21T09:10:23Z

packages/ts/react-crud/src/header-filter.tsx

+   * Allows to set custom filter for a property. This is used by the header filter components.
+   * @param filter - The filter to set in the filter list.
+   */
+  setPropertyFilter(filter: FilterUnion): void;


As this accepts a filter union, it can do more than filtering single properties. Maybe we should rename this to setFilter. Internally we could use setColumnFilter as that's what it basically does now.

Fixes #1608, #1501 *Provide headerFilterRender for users as a new property in columnOptions *Split headerRenderer and headerFilterRenderer to solve consistency issue with headerRenderer being responsible for two places. *Provide setPropertyFilter function to headerFilterRenderer as a property in columnOptions *Add identifier to andFilter and orFilter (key) for controlling (delete/replace/add) filter list *Change setPropertyFilter to accept any Filter not only StringPropertyFilter *Enable adding headerFilterRenderer to custom columns *Enable setting headerRenderer from columnOptions as primary source (overwriting default) *Provide types to users: ColumnOptions, HeaderFilterRendererProps

…filter.tsx

…r for users

…lter working Rename InternalHeaderFilterRenderer to HeaderFilterWrapper

sonarcloud · 2023-12-23T15:16:57Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
No data about Coverage
1.3% Duplication on New Code

See analysis details on SonarCloud

sissbruecker reviewed Dec 21, 2023

View reviewed changes

krissvaa added 9 commits December 23, 2023 17:15

Fix unused imports and constance issues from sonar

2d0d63d

Fix formatting

ac180a9

Simplify/Refactor InternalHeaderFilterRenderer and move it to header-…

4ad3df0

…filter.tsx

Refactor setPropertyFilter to setColumnFilter internally and setFilte…

ad0a66d

…r for users

Move isFilterEmpty function to utils for testing

f86dc22

Remove key from and/or filter and provide alternative for internal fi…

716a401

…lter working Rename InternalHeaderFilterRenderer to HeaderFilterWrapper

Remove key from tests

3d81507

Fix tests

7be51df

krissvaa force-pushed the feat/grid-custom-filters branch from dad84f9 to 7be51df Compare December 23, 2023 15:16

krissvaa requested review from taefi, sissbruecker, platosha and Lodin December 23, 2023 15:17

platosha approved these changes Dec 28, 2023

View reviewed changes

platosha merged commit faa56c8 into main Dec 28, 2023
13 of 15 checks passed

platosha deleted the feat/grid-custom-filters branch December 28, 2023 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(autgrid): Add custom filtering option to columns #1911

feat(autgrid): Add custom filtering option to columns #1911

krissvaa commented Dec 20, 2023

codecov bot commented Dec 20, 2023 •

edited

sissbruecker Dec 21, 2023

krissvaa Dec 23, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sissbruecker Dec 21, 2023

sonarcloud bot commented Dec 23, 2023

feat(autgrid): Add custom filtering option to columns #1911

feat(autgrid): Add custom filtering option to columns #1911

Conversation

krissvaa commented Dec 20, 2023

codecov bot commented Dec 20, 2023 • edited

Codecov Report

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

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

sonarcloud bot commented Dec 23, 2023

Quality Gate passed

codecov bot commented Dec 20, 2023 •

edited