-
Notifications
You must be signed in to change notification settings - Fork 86
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
Fixed bug where two scrollbars showed up on diffs page #296
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.
This pull request has been automatically deployed to FeaturePeek. 👌
Your deployment will be kept up-to-date with this pull request's latest changes.
Please read our docs for more configuration details.
Nice I could use featurepeak to test and it worked! |
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.
Nice catch! However, I don't think the solution here is to detect and swap classes, but instead to change the CSS layout for the list of endpoints to not need it's own scroll container: it's the "main" bit of content on the page, so should be in the main scrolling context of the page.
This is notoriously tricky in CSS, but since Flexbox a lot more doable. I wouldn't blame you if you left that kind of thing for me to tackle, but if you're keen to give it a go, have a look at the Testing Dashboard page. It has the same essential layout (scrollable sidebar + main content). After an initial layout technique, I faced the same problem as you describe here, and changed how the flex box layout worked.
This reverts commit 3b201bb.
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.
Bye bye Javascript, hello CSS-only fix. Great one! There's some tiny nit-picks left over, very gradual improvements, but nothing that should keep this from getting merged.
@@ -260,7 +257,8 @@ const useStyles = makeStyles((theme) => ({ | |||
position: 'fixed', | |||
width: 'inherit', | |||
height: '100vh', | |||
overflowY: 'scroll', | |||
overflowY: 'visible', | |||
overflowX: 'hidden', |
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.
Is there any thing specific that made you hide overflow on the x-axis? Unless there's a specific reason for it, I tend to stay away form it, as it prevents a whole bunch of enhancements to be made. The most common enhancement I see, especially in navigation, is rendering tooltips over links, buttons, etc. Without hiding the overflow, these tooltips would be allowed to render outside the navigation, overlapping some of main page content (which for a tooltip is fine). When overflow is hidden, stuff gets a lot more complicated.
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 had hidden it because it was adding an unnecessary scrollbar — switched it to visible so that if we need to scroll, it'll be able to scroll (https://www.loom.com/share/c0d79ac07dba4f7280bb63f10a0297f8)
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.
Hmm, that's fair enough. It's an indicator of an underlying problem, as that left nav shouldn't overflow in the first place: it should be the set width that it is and all content should shrink to fit or wrap to new lines. I wonder if there's a missing flex-shrink: 0
would fix things, but chances are it's not just that. Don't bother too much with that though, unless you think it's fun.
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.
Nice one Ronak, all good to go!
On firefox/chrome, whenever you went to the diffs page, you would see 2 scrollbars. This was because
CaptureManagerPage
rendered things inside its own container, making it so it had its own overflow — thus adding another scrollbar to be rendered.I fixed this by conditionally hiding the outer scrollbar when you are on the diffs page. It doesn't appear to affect / hide anything, as all elements for the diffs page are rendered within the
CaptureManagerPage
.