Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
BrunoQuaresma merged 7 commits intomainfrombq/fix-build-logs-scrolling
Oct 3, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

Copy link
Member

@ParkreinerParkreiner left a 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

BrunoQuaresma reacted with thumbs up emoji
<div css={{ height: "100%", overflowY: "auto", width: "100%" }}>
<div
ref={contentRef}
css={{ height: "100%", overflowY: "auto", width: "100%" }}
Copy link
Member

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

Copy link
Member

@ParkreinerParkreinerSep 30, 2024
edited
Loading

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%"}}

Comment on lines 88 to 90
if (!build) {
return <Loader />;
}
Copy link
Member

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:

  1. Component mounts with no build
  2. We set up the ref and effect to run after the first render
  3. The first render completes, but because there's no build, we return out a loader and don't attach the ref to anything
  4. The layout effect runs and whiffs
  5. We re-render at some point with a build, but because the layout effect dependency array is empty, nothing in it re-runs
  6. 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

Copy link
Member

@ParkreinerParkreiner left a 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%");
Copy link
Member

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"]>?

Copy link
CollaboratorAuthor

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 🤦

@BrunoQuaresmaBrunoQuaresma merged commit52f03db intomainOct 3, 2024
27 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/fix-build-logs-scrolling branchOctober 3, 2024 16:27
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 3, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@ParkreinerParkreinerParkreiner approved these changes

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Workspace build log scrolling doesn't work in Safari
2 participants
@BrunoQuaresma@Parkreiner

[8]ページ先頭

©2009-2025 Movatter.jp