Performance tuning #1274
Performance tuning #1274
Conversation
|
✔️ Deploy Preview for evergreen-storybook ready! 🔨 Explore the source changes: 0721365 🔍 Inspect the deploy log: https://app.netlify.com/sites/evergreen-storybook/deploys/60f8e804d8c2b400084d6145 😎 Browse the preview: https://deploy-preview-1274--evergreen-storybook.netlify.app |
| <Popover | ||
| bringFocusInside={false} | ||
| isShown={isShown} | ||
| minWidth={popoverMinWidth} | ||
| position={position || (targetWidth < popoverMinWidth ? Position.BOTTOM_LEFT : Position.BOTTOM)} | ||
| content={() => ( |
There was a problem hiding this comment.
Unnecessary new function created every time. Unless we are using the close method we can simply pass the JSX directly
| useIsomorphicLayoutEffect(() => { | ||
| ref.current = value | ||
| }) |
There was a problem hiding this comment.
This was unnecessary.
| minWidth: Math.max(width, minWidth), | ||
| zIndex | ||
| }), | ||
| [left, top, height, width, minHeight, minWidth, zIndex] |
There was a problem hiding this comment.
I think we may want to consider ripping out some of this measurement/dimension logic in favor of a simple hook. There's a lot of complex logic that has nothing to do with this component in particular.
There was a problem hiding this comment.
200% measurement should definitely be a utility hook
| const update = useCallback(() => { | ||
| function updater() { | ||
| const targetRef = getTargetRef.current() | ||
| if (!targetRef) return | ||
| tableBodyRef.current = getTableBodyRef(targetRef) | ||
|
|
||
| const { | ||
| height: targetHeight, | ||
| left: targetLeft, | ||
| top: targetTop, | ||
| width: targetWidth | ||
| } = targetRef.getBoundingClientRect() | ||
|
|
||
| let calculatedTop | ||
| if (tableBodyRef.current) { | ||
| const bounds = tableBodyRef.current.getBoundingClientRect() | ||
| calculatedTop = Math.min(Math.max(targetTop, bounds.top), bounds.bottom - targetHeight) | ||
| } else { | ||
| calculatedTop = targetTop | ||
| } | ||
|
|
||
| setHeight(targetHeight) | ||
| setWidth(targetWidth) | ||
| setLeft(targetLeft) | ||
| setTop(calculatedTop) | ||
|
|
||
| // recursively run the updater | ||
| latestAnimationFrame.current = requestAnimationFrame(() => updater()) | ||
| } | ||
|
|
||
| // kick off the updater | ||
| updater() | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []) |
There was a problem hiding this comment.
This could be replaced with a hook – given a ref, we recursively find the dimensions using resize observer. Using resize observer would mean no IE support, but I highly doubt we have great IE support already (and we don't support it in the app).
There was a problem hiding this comment.
plus the perf on it is likely much better. im in favor of resize observer
| @@ -62,6 +64,7 @@ const Avatar = memo( | |||
| ) | |||
|
|
|||
| const [imageHasFailedLoading, setImageHasFailedLoading] = useState(false) | |||
| const onError = useCallback(() => setImageHasFailedLoading(true), []) | |||
There was a problem hiding this comment.
this may not be as efficient as one would hope: https://kentcdodds.com/blog/usememo-and-usecallback
There was a problem hiding this comment.
Yeah for sure, might be too aggressive here. I was considering that there is internal state that will cause rerenders, and so we may as well cache the function, but it might be excessive in this case.
| const update = useCallback(() => { | ||
| function updater() { | ||
| const targetRef = getTargetRef.current() | ||
| if (!targetRef) return | ||
| tableBodyRef.current = getTableBodyRef(targetRef) | ||
|
|
||
| const { | ||
| height: targetHeight, | ||
| left: targetLeft, | ||
| top: targetTop, | ||
| width: targetWidth | ||
| } = targetRef.getBoundingClientRect() | ||
|
|
||
| let calculatedTop | ||
| if (tableBodyRef.current) { | ||
| const bounds = tableBodyRef.current.getBoundingClientRect() | ||
| calculatedTop = Math.min(Math.max(targetTop, bounds.top), bounds.bottom - targetHeight) | ||
| } else { | ||
| calculatedTop = targetTop | ||
| } | ||
|
|
||
| setHeight(targetHeight) | ||
| setWidth(targetWidth) | ||
| setLeft(targetLeft) | ||
| setTop(calculatedTop) | ||
|
|
||
| // recursively run the updater | ||
| latestAnimationFrame.current = requestAnimationFrame(() => updater()) | ||
| } | ||
|
|
||
| // kick off the updater | ||
| updater() | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []) |
There was a problem hiding this comment.
plus the perf on it is likely much better. im in favor of resize observer
| minWidth: Math.max(width, minWidth), | ||
| zIndex | ||
| }), | ||
| [left, top, height, width, minHeight, minWidth, zIndex] |
There was a problem hiding this comment.
200% measurement should definitely be a utility hook
Overview
This PR addresses a bunch of small performance gains, especially for components that are used in large numbers (like inside Tables).
There are no breaking or obvious functional changes, but it does fix a few bugs in the stories – like editable cells appearing in the left hand corner of the screen 😬
Screenshots (if applicable)
NA
Documentation