-
Notifications
You must be signed in to change notification settings - Fork 101
Fix interaction between search and side bars #1159
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
Conversation
src/html_support_files/odoc.css
Outdated
@@ -1253,7 +1274,7 @@ td.def-doc *:first-child { | |||
} | |||
.odoc-toc { | |||
position: unset; | |||
max-height: unset; | |||
max-height: unset !important; |
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.
Maybe it would be better to have the same priority on both the rule setting the max-height
and the one inside the media 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.
/* When a search bar is present, we need the sticky sidebar to be a bit lower,
so `top` is higher */
.odoc-search + * + .odoc-tocs .odoc-toc {
--toc-top: calc(var(--search-bar-height) + 2 * var(--search-padding-top));
max-height: calc(100vh - 2 * var(--toc-top));
top: var(--toc-top)
}
Here is it, the high priority is built in the rule.
The screenshot are produced with the "final fixes" (a1af33c) commit |
Isn't the use of |
Yes, it is an antipattern. I think it would be better to give a specific class to |
I think the solution you mention is the right one, but I am not sure this should be part of this PR. The brittle rule is present since 2.4. |
This is done by simplifying some rules, and by augmenting the specificity of the other via nesting.
src/html_support_files/odoc.css
Outdated
.odoc-toc { | ||
position: unset; | ||
max-height: unset; | ||
} |
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.
What is this rule doing in another one? Am I too old and this is valid?
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.
Yes, it is valid, and quite cool: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_nesting/Using_CSS_nesting
Thanks for teaching me that!
However, caniuse reports only 82% of used browser with full support. Since this was not used before, is it really worth introducing it?
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 was used before, the whole media rule was nested.
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.
Media queries are not CSS nesting... They have 98% support on caniuse.
src/html_support_files/odoc.css
Outdated
.odoc-tocs { | ||
&:has(> .odoc-search:focus-within) { | ||
/* This is the same as when there is no focus on the search bar, this is | ||
just to prevent the full screen rule from changing anything. */ |
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.
What do you mean with "fullscreen rule"?
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 meant the large screen/default rule
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.
just to prevent the full screen rule from changing anything. */ | |
just to prevent the default "wide layout" rule from changing anything. */ |
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 indeed fixes interaction between search and sidebars.
There are now a lot of bars in odoc, which can be side, search or absent or present.
This fixes weird css interactions in with some combinations in that were merged before we had a chance to fix them in the sidebar pr.