Skip to content
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

fix: scrollshadow chromatic diff issue #1082

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

blambillotte
Copy link
Contributor

@blambillotte blambillotte commented Oct 18, 2024

Overview

One source of flakey chromatic diffs trace back to the use of the ScrollShadows component where the bottom scroll "shadow" element isn't reliably drawn. This generally happens when more complex children components are rendered that may take multiple paints to render the full content. This common flaky example has a GridTable nested inside:
image

When recording how the browser actually paints the component, you can see the initial frames do not include the GridTable (in the 2nd "card" from the top) so the ScrollShadows component doesn't think a bottom shadow is needed.
Initial initial paint does not include the full table
image

Then on subsequent frames, the table fully renders
image

@blambillotte blambillotte marked this pull request as ready for review October 18, 2024 18:36
@@ -63,6 +63,11 @@ export function ScrollShadows(props: ScrollShadowsProps) {
const onResize = useCallback(() => scrollRef.current && updateScrollProps(scrollRef.current), []);
useResizeObserver({ ref: scrollRef, onResize });

// Hack for sizing more complicated components that might not be fully drawn on initial paint (such as a GridTable)
useLayoutEffect(() => {
setTimeout(() => onResize(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, but why the setTimeout instead of invoking onResize immediately? Maybe worth adding to as an in-source comment? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'm not loving the solution - this hack allows us to delay the execution until the current execution thread is completed by placing this callback into the async queue (but without additional delay) since we need the multiple paints to complete before actually trying to size.
A cleaner option perhaps is to just use a normal useEffect which should be called after render, but also not provide any dependencies so it's called after each render since the callback is cheap? 🤔 Doing more testing on that path...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ehhh didn't seem to work on the last chromatic build :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw my has been something in useSetupColumnSizes i.e. this guy:

  // Used to recalculate our columns sizes when columns change
  useEffect(
    () => {
      if (!calculateImmediately.current) {
        const width = resizeRef.current?.clientWidth;
        width && setTableAndColumnWidths(width);
      }
    },
    // TODO: validate this eslint-disable. It was automatically ignored as part of https://app.shortcut.com/homebound-team/story/40033/enable-react-hooks-exhaustive-deps-for-react-projects
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [columns, setTableAndColumnWidths],
  );

I also thought we had a "ruler" div in GridTable somewhere, but I'm not seeing it anymore/atm... 🤔

@blambillotte blambillotte marked this pull request as draft November 6, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants