- Notifications
You must be signed in to change notification settings - Fork928
fix(site): fix build logs scrolling on safari#14884
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
The overall approach makes sense to me – my worry is that I don't think the code is set up 100% correctly, and it won't address all problems that could pop up
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 makinga 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
ParkreinerSep 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I think I'd do something like this:
const[contentHeight,setContentHeight]=useState<string>()constcontentRef=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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
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.
Looks good! Thanks for bearing with me
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Tiny thing, but could we set the type to be based onCSSProperties["height"]>
?
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.
lol yes, idk why I used width 🤦
52f03db
intomainUh oh!
There was an error while loading.Please reload this page.
Fix#9687