Use grid layout for rows, split column metrics from compute columns #2272

nstepien · 2021-01-11T17:27:13Z

What's the fun in making a grid component if you don't use display: grid;?

nstepien · 2021-01-12T15:54:43Z

src/hooks/useViewportColumns.ts

-        column.frozen = true;
-        column.rowGroup = true;
-      }
+    const columns = rawColumns.map(rawColumn => {


We only create 1 columns array now in this useMemo from 2 arrays before.
We also create the column object once, no more intermediary column object.

nstepien · 2021-01-12T15:58:33Z

src/hooks/useViewportColumns.ts

+
+      if (column.rowGroup) {
+        groupBy.push(column.key);
+        column.groupFormatter ??= ToggleGroupFormatter;


Browsers already support this syntax, and babel transpiles it. 👍
https://caniuse.com/mdn-javascript_operators_logical_nullish_assignment
Bonus point: it doesn't re-assign the value if it already exists.
https://v8.dev/blog/v8-release-85#logical-assignment-operators

nstepien · 2021-01-12T15:59:43Z

src/hooks/useViewportColumns.ts

+    };
+  }, [rawColumns, defaultFormatter, defaultResizable, defaultSortable, rawGroupBy]);
+
+  const { layoutCssVars, totalColumnWidth, totalFrozenColumnWidth, columnMetrics } = useMemo(() => {


We now compute column metrics in a separate useMemo, and store it in a separate Map.
That way we can, for example, resize columns without re-creating the column objects, and reducing re-renders.

nstepien · 2021-01-12T16:07:15Z

src/hooks/useViewportColumns.ts

-      totalColumnWidth: totalWidth,
-      groupBy
+    const layoutCssVars: Record<string, string> = {
+      '--template-columns': templateColumns


This is used for the grid-template-columns css property in all rows now.
That way we set the template on the root, and we don't have to update all rows/cells (in js).

nstepien · 2021-01-12T16:08:50Z

src/hooks/useViewportColumns.ts

+
+    for (let i = 0; i <= lastFrozenColumnIndex; i++) {
+      const column = columns[i];
+      layoutCssVars[`--frozen-left-${column.key}`] = `${columnMetrics.get(column)!.left}px`;


The frozen columns still need to know their left position otherwise position: sticky won't work.

nstepien · 2021-01-12T16:10:19Z

src/utils/columnUtils.ts

@@ -1,37 +1,3 @@
-import type { CalculatedColumn } from '../types';
-
-export function getColumnScrollPosition<R, SR>(columns: readonly CalculatedColumn<R, SR>[], idx: number, currentScrollLeft: number, currentClientWidth: number): number {


This did way too much unnecessary work.

nstepien · 2021-01-12T16:11:30Z

src/utils/index.ts

+export function getCellStyle<R, SR>(column: CalculatedColumn<R, SR>): React.CSSProperties {
+  return column.frozen
+    ? { left: `var(--frozen-left-${column.key})` }
+    : { gridColumnStart: column.idx + 1 };


This "picks" the correct grid column from the grid-template-columns, regardless of its DOM position.

nstepien · 2021-01-12T16:12:42Z

style/cell.less

  contain: strict;
  contain: size layout style paint;
-  position: absolute;
-  height: inherit;


Height is set by the grid layout now.

nstepien · 2021-01-12T16:13:52Z

style/row.less

 .rdg-row {
  contain: strict;
  contain: size layout style paint;
-  display: flex;


We know that flex+sticky position cells can be buggy in Firefox, but I wonder if grid+sticky works fine or not.

nstepien · 2021-01-12T16:15:00Z

style/row.less

  contain: size layout style paint;
-  display: flex;
+  display: grid;
+  grid-template-rows: var(--row-height);


This basically means that we only have 1 row in this grid.

amanmahajan7

Looks great overall

src/utils/columnUtils.ts

amanmahajan7 · 2021-01-13T17:55:38Z

src/hooks/useViewportColumns.ts

+    }
+
+    const unallocatedWidth = viewportWidth - allocatedWidth;
    const unallocatedColumnWidth = Math.max(


Do we need to use Math.max if we are using clampColumnWidth later?

We don't, I've removed it now, we also don't need to floor it, now columns will always fill all the available space when possible.
For a before/after comparison, open the columns reordering story, and resize the browser, you'll see that with Math.floor() there can be some space between the last column and the scrollbar.

amanmahajan7 · 2021-01-13T18:13:22Z

src/types.ts


 export interface CalculatedColumn<TRow, TSummaryRow = unknown> extends Column<TRow, TSummaryRow> {
  idx: number;
-  width: number;


I am not sure if any formatter/editor uses width/left or not. What is the alternative if those values are needed? ref?

We could pass them in editorProps if it's ever needed.

… Firefox

Use grid layout for rows

bf3661c

nstepien self-assigned this Jan 11, 2021

nstepien added 3 commits January 11, 2021 18:18

Fix getCellStyle type

e84ecfc

Split column and column metrics

9aeb219

sticky -> left

6d5ce5d

nstepien commented Jan 12, 2021

View reviewed changes

nstepien marked this pull request as ready for review January 12, 2021 16:21

nstepien requested a review from amanmahajan7 January 12, 2021 16:21

nstepien changed the title ~~Use grid layout for rows~~ Use grid layout for rows, split column metrics from compute columns Jan 12, 2021

amanmahajan7 reviewed Jan 13, 2021

View reviewed changes

nstepien added 3 commits January 13, 2021 18:29

Allow decimals in column widths, also fix resiverobserver handling in…

348b639

… Firefox

move onEditorNavigation to keyboardUtils

d91c4a3

-width

d54a8e1

amanmahajan7 approved these changes Jan 13, 2021

View reviewed changes

nstepien merged commit b5de11b into canary Jan 13, 2021

nstepien deleted the gridlayout branch January 13, 2021 19:29

		@@ -1,37 +1,3 @@
		import type { CalculatedColumn } from '../types';

		export function getColumnScrollPosition<R, SR>(columns: readonly CalculatedColumn<R, SR>[], idx: number, currentScrollLeft: number, currentClientWidth: number): number {

Use grid layout for rows, split column metrics from compute columns #2272

Use grid layout for rows, split column metrics from compute columns #2272

Uh oh!

Conversation

nstepien commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstepien Jan 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

nstepien Jan 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanmahajan7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nstepien commented Jan 11, 2021 •

edited

Loading

nstepien Jan 12, 2021 •

edited

Loading

nstepien Jan 12, 2021 •

edited

Loading