rm width/height props, use ResizeObserver to get the grid's content dimensions #2130

nstepien · 2020-08-25T12:05:37Z

I've only added classNames to steer devs toward using proper css sizing rather than more js-heavy alternatives.
We can always add support for style={{ width, height }} if there is demand for it.

…ensions of the grid

nstepien · 2020-08-25T12:07:06Z

src/hooks/useGridDimensions.ts

+  const [gridWidth, setGridWidth] = useState(1);
+  const [gridHeight, setGridHeight] = useState(1);


I've set to 1 by default just to avoid divisions by zero possibly breaking things.

nstepien · 2020-08-25T12:09:06Z

src/hooks/useGridDimensions.ts

+  const [gridHeight, setGridHeight] = useState(1);
+
+  useLayoutEffect(() => {
+    const { ResizeObserver } = window as typeof window & { ResizeObserver: ResizeObserver };


I think it's fair to ask devs to use a polyfill to get the grid working, though that'd only be on IE11/old Edge/Safari 13.
Chrome and Firefox have had it for a long time now.

May be we can add a note on the readme file that a polyfill is required for old browsers. Something like https://reactjs.org/docs/javascript-environment-requirements.html

I've added a line in the readme.

nstepien · 2020-08-25T12:10:09Z

src/hooks/useGridDimensions.ts

+    const { ResizeObserver } = window as typeof window & { ResizeObserver: ResizeObserver };
+
+    const resizeObserver = new ResizeObserver(entries => {
+      const { width, height } = entries[0].contentRect;


We can now properly support css-only dimensions, custom border sizes for the grid, and custom scrollbar sizes as well.

nstepien · 2020-08-25T12:11:45Z

src/utils/domUtils.ts

-
-    document.body.appendChild(scrollDiv);
-    size = scrollDiv.offsetWidth - scrollDiv.clientWidth;
-    document.body.removeChild(scrollDiv);


We won't manually trigger layout/reflow in the middle of rendering now 👌 👌 👌

@nstepien
Could you explain a little bit why we needed this originally? Is it for the scenario when the scroll bar appears and we need to get the new demension?

We needed it to know what the scrollbar size was so we could correctly calculate the actual vertical/horizontal viewport.

nstepien · 2020-08-25T12:12:59Z

style/core.less

@@ -4,6 +4,7 @@
  // https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Positioning/Understanding_z_index/The_stacking_context
  // We set a stacking context so internal elements don't render on top of external components.
  contain: strict;
+  height: 350px;


Same default as before, users can override however they want with height/flex/grid/etc.

amanmahajan7

This is awesome. A few tiny comments. @qili26 do you want to take a look as well?

amanmahajan7 · 2020-08-25T13:03:37Z

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## `alpha` to `canary`
 - **Added:**
  - **Props:**
+    - `className`


Do we want to support the style prop?

Let's add it if there's demand for it, see my opening comment.

amanmahajan7 · 2020-08-25T13:20:27Z

src/hooks/useGridDimensions.ts

+    const { ResizeObserver } = window as typeof window & { ResizeObserver: ResizeObserver };
+
+    // don't break in jest/jsdom and browsers that don't support ResizeObserver
+    if (ResizeObserver == null) return;


So in this case we render an empty grid? I wonder if we should throw an exception

Almost.

We could log something with console.error, but it'd spam jest tests as well.

amanmahajan7 · 2020-08-25T13:29:42Z

src/hooks/useGridDimensions.ts

+  const [gridHeight, setGridHeight] = useState(1);
+
+  useLayoutEffect(() => {
+    const { ResizeObserver } = window as typeof window & { ResizeObserver: ResizeObserver };


May be we can add a note on the readme file that a polyfill is required for old browsers. Something like https://reactjs.org/docs/javascript-environment-requirements.html

amanmahajan7 · 2020-08-25T13:35:56Z

src/utils/domUtils.ts

-
-    document.body.appendChild(scrollDiv);
-    size = scrollDiv.offsetWidth - scrollDiv.clientWidth;
-    document.body.removeChild(scrollDiv);


ekeuus · 2020-10-09T15:31:55Z

So there is no way of passing in the height variable as a prop? I am using css modules and seems like there is no way to really have a full height container anymore.

nstepien · 2020-10-09T15:36:02Z

Use the className or style prop.

rm width/height props, use ResizeObserver to get both the content dim…

954688c

…ensions of the grid

nstepien self-assigned this Aug 25, 2020

nstepien commented Aug 25, 2020

View reviewed changes

fix tests

8bd664f

nstepien marked this pull request as ready for review August 25, 2020 12:20

nstepien requested a review from amanmahajan7 August 25, 2020 12:21

nstepien changed the title ~~rm width/height props, use ResizeObserver to get both the content dimensions of the grid~~ rm width/height props, use ResizeObserver to get the grid's content dimensions Aug 25, 2020

amanmahajan7 reviewed Aug 25, 2020

View reviewed changes

Add a note in the readme

1b018c5

amanmahajan7 approved these changes Aug 25, 2020

View reviewed changes

nstepien merged commit e5d6657 into canary Aug 25, 2020

nstepien deleted the ns-dimensions branch August 25, 2020 14:05

amanmahajan7 mentioned this pull request Oct 2, 2020

Table height continuously growing #2165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rm width/height props, use ResizeObserver to get the grid's content dimensions #2130

rm width/height props, use ResizeObserver to get the grid's content dimensions #2130

nstepien commented Aug 25, 2020 •

edited

Loading

nstepien Aug 25, 2020

nstepien Aug 25, 2020 •

edited

Loading

amanmahajan7 Aug 25, 2020

nstepien Aug 25, 2020

nstepien Aug 25, 2020

nstepien Aug 25, 2020

amanmahajan7 Aug 25, 2020

qili26 Aug 25, 2020

nstepien Aug 25, 2020

nstepien Aug 25, 2020

amanmahajan7 left a comment •

edited

Loading

amanmahajan7 Aug 25, 2020

nstepien Aug 25, 2020

amanmahajan7 Aug 25, 2020

nstepien Aug 25, 2020

amanmahajan7 Aug 25, 2020

amanmahajan7 Aug 25, 2020

ekeuus commented Oct 9, 2020

nstepien commented Oct 9, 2020

		const [gridWidth, setGridWidth] = useState(1);
		const [gridHeight, setGridHeight] = useState(1);

rm width/height props, use ResizeObserver to get the grid's content dimensions #2130

rm width/height props, use ResizeObserver to get the grid's content dimensions #2130

Conversation

nstepien commented Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

nstepien Aug 25, 2020 • edited Loading

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

amanmahajan7 left a comment • edited Loading

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

ekeuus commented Oct 9, 2020

nstepien commented Oct 9, 2020

nstepien commented Aug 25, 2020 •

edited

Loading

nstepien Aug 25, 2020 •

edited

Loading

amanmahajan7 left a comment •

edited

Loading