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

[ekeuus]

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:

1. width = 100
2. width = 107
3. 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[bot]]

Codecov Report

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

:exclamation: 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%)	:arrow_down:

[nstepien]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[ekeuus]

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

[nstepien]

https://github.com/adazzle/react-data-grid/commit/625fad550d7457612bea00af668787711fd3f518 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.