-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rm width/height props, use ResizeObserver to get the grid's content dimensions #2130
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
export * from './useCombinedRefs'; | ||
export * from './useClickOutside'; | ||
export * from './useGridWidth'; | ||
export * from './useGridDimensions'; | ||
export * from './useViewportColumns'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { useRef, useState, useLayoutEffect } from 'react'; | ||
|
||
// https://github.com/microsoft/TypeScript/issues/37861 | ||
interface ResizeObserverEntry { | ||
contentRect: { | ||
width: number; | ||
height: number; | ||
}; | ||
} | ||
|
||
type ResizeObserver = new (callback: (entries: readonly ResizeObserverEntry[]) => void) => { | ||
observe: (target: Element) => void; | ||
disconnect: () => void; | ||
}; | ||
|
||
export function useGridDimensions(): [React.RefObject<HTMLDivElement>, number, number] { | ||
const gridRef = useRef<HTMLDivElement>(null); | ||
const [gridWidth, setGridWidth] = useState(1); | ||
const [gridHeight, setGridHeight] = useState(1); | ||
Comment on lines
+18
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've set to |
||
|
||
useLayoutEffect(() => { | ||
const { ResizeObserver } = window as typeof window & { ResizeObserver: ResizeObserver }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a line in the readme. |
||
|
||
// don't break in jest/jsdom and browsers that don't support ResizeObserver | ||
if (ResizeObserver == null) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in this case we render an empty grid? I wonder if we should throw an exception There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
const resizeObserver = new ResizeObserver(entries => { | ||
const { width, height } = entries[0].contentRect; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can now properly support css-only dimensions, custom border sizes for the grid, and custom scrollbar sizes as well. |
||
setGridWidth(width); | ||
setGridHeight(height); | ||
}); | ||
|
||
resizeObserver.observe(gridRef.current!); | ||
|
||
return () => { | ||
resizeObserver.disconnect(); | ||
}; | ||
}, []); | ||
|
||
return [gridRef, gridWidth, gridHeight]; | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,3 @@ | ||
let size: number | undefined; | ||
|
||
export function getScrollbarSize(): number { | ||
if (size === undefined) { | ||
const scrollDiv = document.createElement('div'); | ||
|
||
scrollDiv.style.position = 'absolute'; | ||
scrollDiv.style.top = '-9999px'; | ||
scrollDiv.style.width = '50px'; | ||
scrollDiv.style.height = '50px'; | ||
scrollDiv.style.overflow = 'scroll'; | ||
|
||
document.body.appendChild(scrollDiv); | ||
size = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
document.body.removeChild(scrollDiv); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We won't manually trigger layout/reflow in the middle of rendering now 👌 👌 👌 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nstepien There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We needed it to know what the scrollbar size was so we could correctly calculate the actual vertical/horizontal viewport. |
||
} | ||
|
||
return size; | ||
} | ||
|
||
export function preventDefault(event: React.SyntheticEvent) { | ||
event.preventDefault(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support the style prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add it if there's demand for it, see my opening comment.