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

Partially fix VerticalScrollOverflowView #17

Merged

Conversation

taneliang
Copy link

Summary

Following up on https://twitter.com/brian_d_vaughn/status/1422034168612102147!

I've fixed some of the major scrolling issues, but it's still a little incomplete. Making a PR with what I have so far to your branch as it's getting late. I can continue fixing the known issues in a future PR.

Known issues:

  1. Time axis markers are hidden by scrolled content. We'll probably want to fix this by
  2. The scrollbar's height doesn't update correctly when resizing, but it'll be corrected when you move your cursor after resizing. I'm not too sure why this is happening yet, but it seems like _contentView's desiredSize() isn't returning the new height.

This PR also targets your repo and branch since it's based off your stashed code – let me know if I should retarget this upstream :)

Test Plan

Before

Screen.Recording.2021-08-09.at.10.30.41.PM.mov

After

Screen.Recording.2021-08-09.at.10.09.27.PM.mov

…positioned at y=0

This layouter computed its remainingHeight value assuming its superview was
always positioned at y=0. This caused issues because views are scrolled by
changing their frame's origin.y value, as well as all their subviews'.
@bvaughn
Copy link
Owner

bvaughn commented Aug 9, 2021

Oh great work, @taneliang! Thanks for working on this 😁

I've also got a PR (facebook#22043) that moves some of the view state up into the context so it's remembered if you tab out and back in but this PR shouldn't conflict with that at all since this view is new.

I think this is really the last thing blocking the initial launch of the profiler to the 18 working group so I'm excited. I'll review it this morning!

@bvaughn
Copy link
Owner

bvaughn commented Aug 9, 2021

Actually, I'll just merge it as-is to my branch since it's clearly an improvement over the functionality I had 😁 I'll see if I can get it landed into main today.

@bvaughn bvaughn merged commit 91e1b1b into bvaughn:devtools-scheduling-profiler Aug 9, 2021
@taneliang
Copy link
Author

Thank you! Loving the improvements so far – it feels so much more complete than what we had at the end of last summer. Can't wait to see what people think 😄

If the scrollbar isn't urgent yet, I can continue fixing the remaining issues over the coming week. Let me know!

@taneliang taneliang deleted the fix-scrollbar branch August 9, 2021 15:31
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