-
Notifications
You must be signed in to change notification settings - Fork 512
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
fix(sidebar): fix scrolling behavior #8512
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.
I can imagine the intention of those changes, but it would be good if the commit message would list the different changes and what they do / why they are necessary:
- You pull the CSS variables up.
- You restrict the
overflow: auto
to screens with low height. - You change the
.toc-container
fromoverflow: scroll
tooverflow: auto
. - You change the
.sidebar-inner
from Flexbox to Grid for low height, and Flexbox for sufficient height. - You change the
.sidebar-inner-nav
todisplay: contents
for low height, andblock
for sufficient height. - You introduce a
$screen-height-limit
to distinguish low and sufficient height.
client/src/ui/_vars.scss
Outdated
@@ -247,6 +247,8 @@ $screen-lg: 992px; | |||
$screen-xl: 1200px; | |||
$screen-xxl: 1441px; | |||
|
|||
$screen-height-limit: 44rem; |
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.
Can you add a comment for what this is? Or maybe you find a more descriptive name?
@media screen and (min-height: $screen-height-limit), | ||
screen and (min-width: $screen-lg) { |
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.
For consistency, can we put the min-width
condition first? If stylelint
auto-fixes this, let's move the min-height
condition to an inner block.
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.
Did get the last part? Move to what inner block?
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.
Basically the idea was either:
@media screen and (min-width: $screen-lg), screen and (min-height: $screen-height-limit) {
/* ... */
}
Or (if the order was forced by stylelint):
@media screen and (min-width: $screen-lg) {
@media screen and (min-height: $screen-height-limit) {
/* ... */
}
}
client/src/document/index.scss
Outdated
@media screen and (max-width: $screen-lg) { | ||
.mdn-cta-container ~ .document-page & { | ||
--offset: calc(var(--main-document-header-height) + 3rem); | ||
--max-height: calc(100vh - var(--offset)); |
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.
Actually I don't think we need to override --max-height
, do we?
- .sidebar-inner is a grid when < screen-md - this allows to use display: contents on .sidebar-inner-nav and more the .place up via grid-row: 2/3 - screen-height-place-limit introduced to have central place for it - on screens > screen-md and height < screen-height-place-limit: use .use display flex on .sidebar-inner and block on sidebar-inner nav to revert the gird again - .toc-container: overflow: auto to remove scroll bars
Summary
Fixes #8494
Problem
Several issues.
Solution
Set overflow correct.
Screenshots
Before
After