Grid : Enable sortable on individual columns by kkakroo · Pull Request #28633 · microsoft/fluentui

kkakroo · 2023-07-24T23:09:07Z

Previous Behavior

The sortable property can be added to the Grid which makes each column of the Grid sortable.

The sorting happens on the compare function which is provided as part of columns prop.
If sortable is present and compare function is not provided for a column, the sorting UI changes occur(arrows and hover behavior) but the sorting does not happen for that column.

There is no way currently to enable sorting on specific columns.

New Behavior

If we add a compare function to a particular column in the columns prop, the column becomes sortable automatically.
This helps enable sorting on specific columns without setting the sortable prop.

The previous sortable prop behaviors are as is.

Related Issue(s)

Fixes [Feature]: DataGrid v9 - ability to set sortable property on individual columns #28511

fabricteam · 2023-07-24T23:18:15Z

Perf Analysis (`@fluentui/react-components`)

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
InfoButton	mount	16	14	5000	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	608	626	5000
Button	mount	304	304	5000
Field	mount	1093	1105	5000
FluentProvider	mount	672	678	5000
FluentProviderWithTheme	mount	81	84	10
FluentProviderWithTheme	virtual-rerender	61	67	10
FluentProviderWithTheme	virtual-rerender-with-unmount	74	76	10
InfoButton	mount	16	14	5000	Possible regression
MakeStyles	mount	832	856	50000
Persona	mount	1723	1658	5000
SpinButton	mount	1378	1379	5000

fabricteam · 2023-07-24T23:20:00Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-table` DataGrid	`156.416 kB` `43.567 kB`	`156.78 kB` `43.721 kB`	`364 B` `154 B`
`react-table` Table (Primitives only)	`42.463 kB` `13.255 kB`	`42.48 kB` `13.279 kB`	`17 B` `24 B`
`react-table` Table as DataGrid	`129.276 kB` `34.739 kB`	`129.352 kB` `34.801 kB`	`76 B` `62 B`
`react-table` Table (Selection only)	`74.56 kB` `20.053 kB`	`74.636 kB` `20.108 kB`	`76 B` `55 B`
`react-table` Table (Sort only)	`73.191 kB` `19.649 kB`	`73.267 kB` `19.704 kB`	`76 B` `55 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-components` react-components: Button, FluentProvider & webLightTheme	`69.328 kB` `19.608 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`208.141 kB` `59.349 kB`
`react-components` react-components: FluentProvider & webLightTheme	`40.713 kB` `13.509 kB`
`react-portal-compat` PortalCompatProvider	`6.541 kB` `2.227 kB`

_{🤖 This report was generated against 634979cbc69ae97e39baf2f4143dab6caa3a6968}

packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx

packages/react-components/react-table/stories/DataGrid/Default.stories.tsx

packages/react-components/react-table/src/components/DataGridBody/useDataGridBody.tsx

sopranopillow · 2023-08-21T20:39:57Z

To pass the error you need to run yarn buildto @fluentui/react-table to generate the api files.

sopranopillow · 2023-08-21T20:59:09Z

Hi @kkakroo, just reaching out to see if you would like to keep working on this PR?

codesandbox-ci · 2023-08-22T20:16:08Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1e2f2a3:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2023-08-22T20:24:33Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 634979cbc69ae97e39baf2f4143dab6caa3a6968 (build)

sopranopillow · 2023-08-23T16:58:22Z

@kkakroo the change file and api files are missing, to fix this you have to:

run yarn buildto @fluentui/react-components
commit your changes
run yarn change
commit your change files
push

Also, would you mind updating the description to describe the changes?

kkakroo · 2023-08-23T21:33:15Z

Hi @sopranopillow, made the requested changes as well, thanks for letting me know.
Please have a look at my comment if you have some time. I had a query to complete the missing piece of this feature.

If you think i am not going in the right direction or delaying the feature delivery, feel free to let me know.
I can close this PR and let you take over.

sopranopillow · 2023-08-23T22:04:40Z

Hi @sopranopillow, made the requested changes as well, thanks for letting me know. Please have a look at my comment if you have some time. I had a query to complete the missing piece of this feature.

If you think i am not going in the right direction or delaying the feature delivery, feel free to let me know. I can close this PR and let you take over.

No worries, looks close to mergeable. I'll let the team responsible for this give a final review.

sopranopillow · 2023-08-25T17:17:37Z

@kkakroo looks like the snapshots are outdated, you need to run yarn workspace @fluentui/react-table test -u to update it and push those changes.

kkakroo · 2023-08-25T17:59:59Z

@sopranopillow , thanks for pointing it out. Done.
Saw some tests failing as well, let me have a look into those meanwhile.

fabricteam · 2023-08-25T21:12:01Z

🕵 fluentuiv9 No visual regressions between this PR and main

ling1726 · 2023-09-05T16:12:20Z

packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx

 ): TableHeaderCellState => {
-  const { noNativeElements, sortable } = useTableContext();
+  const { noNativeElements, sortable: contextSortable } = useTableContext();
+  const { sortable = contextSortable } = props;


Can you add a Table example for using this prop on headers?

ling1726 · 2023-09-05T16:13:05Z

packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx

 ): TableHeaderCellState => {
-  const { noNativeElements, sortable } = useTableContext();
+  const { noNativeElements, sortable: contextSortable } = useTableContext();
+  const { sortable = contextSortable } = props;


Please add a test for the prop/context override in TableHeaderCell.test.tsx

ling1726 · 2023-09-05T16:18:08Z

...ages/react-components/react-table/src/components/DataGridHeaderCell/useDataGridHeaderCell.ts

+  let { sortable = false } = props;
  const columnId = useColumnIdContext();
-  const { sortable } = useTableContext();
+  const { sortable: gridSortable } = useTableContext();
  const toggleColumnSort = useDataGridContext_unstable(ctx => ctx.sort.toggleColumnSort);
+  const columns = useDataGridContext_unstable(ctx => ctx.columns);
+  sortable = gridSortable || columns.find(c => c.columnId === columnId)?.compare.length !== 0;


This would avoid returning a non-primitive from the DataGridContext (which avoids a performance regression)

const columnSortable = useDataGridContext_unstable(ctx => ctx.getSortableColumns().find(column => column.columnId === columnId), ); const { sortable: gridSortable } = useTableContext(); const toggleColumnSort = useDataGridContext_unstable(ctx => ctx.sort.toggleColumnSort); const { sortable = gridSortable ?? columnSortable } = props; const sortDirection = useDataGridContext_unstable(ctx => sortable ? ctx.sort.getSortDirection(columnId) : undefined, );

Playing aroud a bit with your PR this might be the best way to go

const isColumnSortable = (columns: TableColumnDefinition<any>[], columnId: TableColumnId) => { return !!columns.filter(c => c.compare.length !== 0 && c.columnId === columnId).length; }; const useColumnSortable = (sortableProp: boolean | undefined) => { const columnId = useColumnIdContext(); const columnSortable = useDataGridContext_unstable(ctx => isColumnSortable(ctx.columns, columnId) ); const { sortable: gridSortable } = useTableContext(); return sortableProp ?? gridSortable ?? columnSortable; }; // useDataGridHeaderCell_unstable useColumnSortable(props.sortable)

ling1726 · 2023-09-05T16:50:25Z

packages/react-components/react-table/src/hooks/useTableFeatures.ts

 ): TableFeaturesState<TItem> {
  const { items, getRowId, columns } = options;

+  const getSortableColumns = () => columns.filter(c => c.compare.length !== 0);


I don't think this needs to be a part of the public API, a simple utility function would do

const isColumnSortable = (columns: TableColumnDefinition<any>[], columnId: TableColumnId) => { return !!columns.filter(c => c.compare.length !== 0 && c.columnId === columnId).length; };

I think any in this case is acceptable since the utility stays internal. I can't quite think of a solid scenario why we would need a way to do this in the public API yet, so I'd rather avoid making it public for now

ling1726 · 2023-09-05T16:53:08Z

packages/react-components/react-table/stories/DataGrid/ColumnSort.stories.tsx

+  );
+};
+
+ColumnSort.parameters = {


Instead of this example, can you just update the current Sort example and remove the sortable prop?

fluentui/packages/react-components/react-table/stories/DataGrid/Sort.stories.tsx

Line 159 in 0cd9cea

<DataGrid items={items} columns={columns} sortable defaultSortState={defaultSortState}>

Then you can add the guidance here that column definitions without a compare function will not be sortable

ling1726 · 2023-09-05T16:53:55Z

@kkakroo Thanks for contributing! sorry about the lead time to this review, I've got this firmly in my sights now, there are few things to address but nothing too major

ling1726 · 2023-09-21T09:53:07Z

Going to take over this work and publish it in #29196

kkakroo · 2023-10-03T17:04:27Z

Sorry for not being able to address the review comments on time, was on vacation.
Thanks for taking this up.

initial commit

b8b8dab

kkakroo added 7 commits July 25, 2023 00:22

Merge branch 'master' into grid-sortable

f014d0b

Merge branch 'master' into grid-sortable

d567171

Merge branch 'master' into grid-sortable

88bd132

Merge branch 'master' into grid-sortable

3e8b93d

Merge branch 'master' into grid-sortable

d718c62

Merge branch 'master' into grid-sortable

65bc0ef

Merge branch 'master' into grid-sortable

3774065

sopranopillow reviewed Aug 21, 2023

View reviewed changes

packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx Outdated Show resolved Hide resolved

packages/react-components/react-table/stories/DataGrid/Default.stories.tsx Outdated Show resolved Hide resolved

sopranopillow reviewed Aug 21, 2023

View reviewed changes

packages/react-components/react-table/src/components/DataGridBody/useDataGridBody.tsx Show resolved Hide resolved

kkakroo added 2 commits August 21, 2023 22:02

Merge branch 'master' into grid-sortable

6a213cd

review comment update

755ece9

sopranopillow assigned kkakroo Aug 21, 2023

kkakroo added 2 commits August 22, 2023 20:58

Merge branch 'master' into grid-sortable

a5a63d2

added api.md file

dafb485

added getSortableColumns

32a908f

kkakroo added 2 commits August 23, 2023 22:23

Merge branch 'master' into grid-sortable

2fb4ffd

updated api.md file

f0f5c61

added ColumnSort story

2f5f181

sopranopillow changed the title ~~Grid: enabling sortable on individual columns~~ fix(react-table): Enable sortable on individual columns Aug 23, 2023

sopranopillow marked this pull request as ready for review August 23, 2023 22:03

sopranopillow requested a review from a team as a code owner August 23, 2023 22:03

kkakroo added 2 commits August 24, 2023 00:22

Merge branch 'master' into grid-sortable

e2dbcb7

Merge branch 'master' into grid-sortable

692d103

kkakroo changed the title ~~fix(react-table): Enable sortable on individual columns~~ Grid : Enable sortable on individual columns Aug 24, 2023

Merge branch 'master' into grid-sortable

bc8142a

kkakroo added 2 commits August 25, 2023 18:51

Merge branch 'master' into grid-sortable

7aed59d

updated snapshots

df04ec8

Merge branch 'master' into grid-sortable

dc90514

kkakroo added 4 commits August 31, 2023 14:05

Merge branch 'master' into grid-sortable

fc54f57

Merge branch 'master' into grid-sortable

1219e93

Merge branch 'master' into grid-sortable

2382e77

Merge branch 'master' into grid-sortable

8daa21e

layershifter requested a review from ling1726 September 4, 2023 14:35

ling1726 reviewed Sep 5, 2023

View reviewed changes

kkakroo added 2 commits September 9, 2023 18:25

Merge branch 'master' into grid-sortable

fd6dc37

updated Sort story and removed Column Sort story

1e2f2a3

ling1726 mentioned this pull request Sep 21, 2023

feat: Individual columns should be sortable #29196

Merged

kkakroo closed this Oct 3, 2023

Comments

Conversation

kkakroo commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Previous Behavior

New Behavior

Related Issue(s)

Uh oh!

fabricteam commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf Analysis (@fluentui/react-components)

Uh oh!

fabricteam commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopranopillow commented Aug 21, 2023

Uh oh!

sopranopillow commented Aug 21, 2023

Uh oh!

codesandbox-ci bot commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

sopranopillow commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkakroo commented Aug 23, 2023

Uh oh!

sopranopillow commented Aug 23, 2023

Uh oh!

sopranopillow commented Aug 25, 2023

Uh oh!

kkakroo commented Aug 25, 2023

Uh oh!

fabricteam commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🕵 fluentuiv9 No visual regressions between this PR and main

Uh oh!

ling1726 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

ling1726 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

ling1726 Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ling1726 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

ling1726 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

ling1726 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

ling1726 commented Sep 5, 2023

Uh oh!

ling1726 commented Sep 21, 2023

Uh oh!

kkakroo commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kkakroo commented Jul 24, 2023 •

edited

Loading

fabricteam commented Jul 24, 2023 •

edited

Loading

Perf Analysis (`@fluentui/react-components`)

fabricteam commented Jul 24, 2023 •

edited

Loading

codesandbox-ci bot commented Aug 22, 2023 •

edited

Loading

size-auditor bot commented Aug 22, 2023 •

edited

Loading

sopranopillow commented Aug 23, 2023 •

edited

Loading

fabricteam commented Aug 25, 2023 •

edited

Loading

ling1726 Sep 5, 2023 •

edited

Loading