🆕 Update width only if it isn't exactly the previous value #2870

ekeuus · 2022-03-31T13:23:28Z

We are having issues with the width sidebar jumping back and forth in some specific cases. The useGridDimensions keeps re-rendering and updating the width in a loop. This PR makes sure that the previous value is also kept in the state so that if there is a state pattern like this:

width = 100
width = 107
width = 100

Then it just doesn't set the width of 100 in step 3 again, preventing the infinite rendering loop. If you resize your screen as a user or snap it via the OS, the table resizing still works fine since there are more events being triggered in that case, compared to the sidebar appearing and disappearing.

WITH TWITCHING BEFORE THIS FIX:
https://user-images.githubusercontent.com/10929022/161064356-d2053799-beac-41fb-a8a2-17b1ba855806.mp4

WITHOUT TWITCHING AFTER THIS FIX:
https://user-images.githubusercontent.com/10929022/161064370-ad11a323-3850-44b9-82dc-b93998acd342.mp4

codecov · 2022-03-31T13:26:07Z

Codecov Report

Merging #2870 (7d156be) into main (cb4c389) will decrease coverage by 0.06%.
The diff coverage is 85.71%.

❗ Current head 7d156be differs from pull request most recent head 96c1751. Consider uploading reports for the commit 96c1751 to get more accurate results

@@            Coverage Diff             @@
##             main    #2870      +/-   ##
==========================================
- Coverage   96.22%   96.15%   -0.07%     
==========================================
  Files          37       37              
  Lines        1244     1248       +4     
  Branches      392      393       +1     
==========================================
+ Hits         1197     1200       +3     
- Misses         47       48       +1

Impacted Files	Coverage Δ
src/hooks/useGridDimensions.ts	`95.23% <85.71%> (-4.77%)`	⬇️

nstepien · 2022-03-31T13:47:54Z

What if for example I have this situation:

grid takes the whole width of the page
I open a sidebar, the grid is now at 80% width of the page
I close the sidebar, the grid goes back to 100% width of the page

Won't this change make the grid use 80% of its actual width?

ekeuus · 2022-03-31T13:58:06Z

What if for example I have this situation:

grid takes the whole width of the page

I open a sidebar, the grid is now at 80% width of the page

I close the sidebar, the grid goes back to 100% width of the page

Won't this change make the grid use 80% of its actual width?

I think in these cases there should be more resize events emitted than just the two? I tried some resizing options and it always worked for me.

Do you think it would make more sense to keep a longer frame in the array? I think it is important to detect the looping pattern and break it if it occurs.

ekeuus · 2022-03-31T13:59:28Z

What if we also keep in a variable the last date of the change so that we allow resize if it happened N seconds ago. The twitching I am seeing happens with a frequency <1s, so if we allow all resizes above that time, it should work.

ekeuus · 2022-03-31T14:10:42Z

I made an experiment with toggling the width between 50% and 100% and it seems like it still works. I am still confident the resizer detects more than just one event in case of resize.

Video:
https://user-images.githubusercontent.com/10929022/161075396-17b117e3-00a2-474e-a358-0c0a22883454.mp4

Code:

	const [width, setWidth] = useState('100%');

	useInterval(() => {
		if (width === '100%') {
			setWidth('50%');
		} else {
			setWidth('100%');
		}
	}, 1000);

	return (
		<div style={{ width }}>
			<ReactDataGrid
				className={styles.table}
				// @ts-ignore
				rows={flatItems}
				// @ts-ignore
				columns={columns}
				// @ts-ignore
				onRowsUpdate={onRowsUpdate}
				rowHeight={ROW_HEIGHT}
				highlightEditable
			/>
		</div>
	);

PS. This using our fork with the commit in this PR - https://www.npmjs.com/package/react-data-grid-planyard

nstepien · 2022-03-31T14:34:43Z

It does bug out when testing with 3 different widths at least.

What's your devicePixelRatio value? Might have to change from -1 to -2.

ekeuus · 2022-03-31T14:45:06Z

It does bug out when testing with 3 different widths at least.

What's your devicePixelRatio value? Might have to change from -1 to -2.

I think it's devicePixelRatio 2

nstepien · 2022-03-31T14:47:55Z

Could you check if this fixes it on your end?

-setGridWidth(clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1));
+setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 1));

or

-setGridWidth(clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1));
+setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 2));

ekeuus · 2022-03-31T14:48:42Z

I could potentially also think of add an additional condition:

if (prev[0] === newValue && Math.abs(prev[0] - newValue) < SOME_FIXED_SCROLLBAR_SIZE)

SOME_FIXED_SCROLLBAR_SIZE can be for example something like 20-50px. This means these larger resizes should also work and the small twitching resizes would be excluded.

ekeuus · 2022-03-31T14:49:56Z

Could you check if this fixes it on your end?

-setGridWidth(clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1));
+setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 1));

or

-setGridWidth(clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1));
+setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 2));

I don't think so. When I log out the variables in the hook, I get the following size changes - the clientWidth drastically changes and it isn't just a single pixel change.

resizeObserver called
instrument.js:109 clientWidth, clientHeight 926 76
instrument.js:109 clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1) 926
instrument.js:109 devicePixelRatio 2
instrument.js:109 resizeObserver called
instrument.js:109 clientWidth, clientHeight 933 83
instrument.js:109 clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1) 933
instrument.js:109 devicePixelRatio 2
resizeObserver called
instrument.js:109 clientWidth, clientHeight 926 76
instrument.js:109 clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1) 926
instrument.js:109 devicePixelRatio 2
instrument.js:109 resizeObserver called
instrument.js:109 clientWidth, clientHeight 933 83
instrument.js:109 clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1) 933
instrument.js:109 devicePixelRatio 2

nstepien · 2022-03-31T14:51:18Z

Please do try my suggestion.

it isn't just a single pixel change.

This is because a single pixel change will toggle the display of the scrollbar.

ekeuus · 2022-03-31T14:59:15Z

setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 2));

This seems to resolve my issue on a devicePixelRatio=2 screen, but I can't reproduce it anyway on a devicePixelRatio=1 screen at the moment tbh.

nstepien · 2022-03-31T15:07:01Z

Let's go ahead with this fix instead then.
Can you confirm that setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 1)); doesn't fix it for you?

nstepien · 2022-04-05T16:08:55Z

@ekeuus Do you want to update this PR to use setGridWidth(clientWidth - (devicePixelRatio === 1 ? 0 : 2)); or should I apply this tweak myself?

ekeuus · 2022-04-05T16:39:40Z

We now released our version to our users, but I can test your solution tomorrow and let you know what I find out.

nstepien · 2022-04-06T19:30:57Z

625fad5
We've ended up using the ResizeObserver's sizes, and tweaked how we handle devicePixelRatio, please let us know if it makes things better/worse/neither.

🆕 Add isMouseClick flag to editable cell to know why the editor i…

ERROR in ./website/root.tsx Module build failed (from ./node_modules/@linaria/webpack5-loader/lib/index.js): Error: Cannot find module '@babel/plugin-proposal-nullish-coalescing-operator' Require stack: ......... - Did you mean "@babel/plugin-transform-nullish-coalescing-operator"? Make sure that all the Babel plugins and presets you are using are defined as dependencies or devDependencies in your package.json file. It's possible that the missing plugin is loaded by a preset you are using that forgot to add the plugin to its dependencies: you can workaround this problem by explicitly adding the missing package to your top-level package.json.

BUG: fix scroll to column

🆕 Update width only if it isn't exactly the previous value

7d156be

ekeuus requested review from amanmahajan7 and nstepien as code owners March 31, 2022 13:23

nstepien mentioned this pull request Apr 6, 2022

ResizeObserver: Use contentBoxSize #2882

Merged

ekeuus and others added 6 commits July 1, 2022 14:02

🆕 Add isMouseClick flag to editable cell to know why the editor is open

65f0fb6

Merge pull request #2 from fizure/feature/add_mouse_click_flag

96c1751

🆕 Add isMouseClick flag to editable cell to know why the editor i…

🐛 Fix scroll to column by providing default value for the rowIdx

5e7008d

⬆️ Bump package version

4ea04e9

Merge pull request #3 from fizure/feature/fix_scroll_to_column

c2a21c0

BUG: fix scroll to column

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🆕 Update width only if it isn't exactly the previous value #2870

🆕 Update width only if it isn't exactly the previous value #2870

ekeuus commented Mar 31, 2022

codecov bot commented Mar 31, 2022 •

edited

Loading

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

ekeuus commented Mar 31, 2022

ekeuus commented Mar 31, 2022 •

edited

Loading

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

ekeuus commented Mar 31, 2022

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

nstepien commented Mar 31, 2022

nstepien commented Apr 5, 2022

ekeuus commented Apr 5, 2022

nstepien commented Apr 6, 2022

🆕 Update width only if it isn't exactly the previous value #2870

Are you sure you want to change the base?

🆕 Update width only if it isn't exactly the previous value #2870

Conversation

ekeuus commented Mar 31, 2022

codecov bot commented Mar 31, 2022 • edited Loading

Codecov Report

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

ekeuus commented Mar 31, 2022

ekeuus commented Mar 31, 2022 • edited Loading

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

ekeuus commented Mar 31, 2022

nstepien commented Mar 31, 2022

ekeuus commented Mar 31, 2022

nstepien commented Mar 31, 2022

nstepien commented Apr 5, 2022

ekeuus commented Apr 5, 2022

nstepien commented Apr 6, 2022

codecov bot commented Mar 31, 2022 •

edited

Loading

ekeuus commented Mar 31, 2022 •

edited

Loading