fix(site): fix build logs scrolling on safari #14884
Conversation
| <div css={{ height: "100%", overflowY: "auto", width: "100%" }}> | ||
| <div | ||
| ref={contentRef} | ||
| css={{ height: "100%", overflowY: "auto", width: "100%" }} |
There was a problem hiding this comment.
If we're going to update the height, I think we have no choice put to put the value in render state
I just tried making a test component to see what would happen if we don't put height changes in state, and it feels like it's too easy to accidentally wipe the adjustments away
There was a problem hiding this comment.
I think I'd do something like this:
const [contentHeight, setContentHeight] = useState<string>()
const contentRef = useRef<HTMLDivElement>(null);
useLayoutEffect(() => {
// It's been a while since I've had to do this, but dispatching a state update from the
// resize callback might actually be too slow, and you might get a UI flicker still. You
// might need to set up the resize callback to dispatch state updates, but then also
// do a one-time direct mutation when the effect first mounts
}, [])
css={{ height: contentHeight || "100%" }}| if (!build) { | ||
| return <Loader />; | ||
| } |
There was a problem hiding this comment.
I think we have an edge case here that could break the effect logic:
- Component mounts with no build
- We set up the ref and effect to run after the first render
- The first render completes, but because there's no build, we return out a loader and don't attach the ref to anything
- The layout effect runs and whiffs
- We re-render at some point with a build, but because the layout effect dependency array is empty, nothing in it re-runs
- The only way to get the size mutations to apply at this point is if the browser resizes, but if the user never resizes anything, we're stuck with the same issue as before
Wondering if might be best to make a new component boundary and move this state there. That way, the state only mounts when we're guaranteed to have a build
| @@ -212,6 +219,7 @@ const ScrollArea: FC<HTMLProps<HTMLDivElement>> = () => { | |||
| // Issue: https://github.com/coder/coder/issues/9687 | |||
| // Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari | |||
| const contentRef = useRef<HTMLDivElement>(null); | |||
| const [height, setHeight] = useState<CSSProperties["width"]>("100%"); | |||
There was a problem hiding this comment.
Tiny thing, but could we set the type to be based on CSSProperties["height"]>?
There was a problem hiding this comment.
lol yes, idk why I used width 🤦
Fix #9687