yari
yari copied to clipboard
fix(sidebar): Update sidebar styling to fix margin-related issues
Summary
This PR fixes a few styling issues caused by the sidebar, as well as performing some slight tweaks. This PR primarily resolves an issue where a long sidebar on just the right screen width will create a bunch of empty space below the page footer.
Problem
On certain screen sizes, there is a nasty margin below the footer of the page, creating lots of scrollable blank space. It turns out that this margin was caused by the size of the navigation bar.
Solution
This removes the height requirements for media query introduced in #8512. The overflow: auto;
that was introduced in #8512 must be applied to the specific screen width regardless of the screen height.
As a small nice-to-have tweak, this also applies the fadeout at the top and bottom of the navbar always, regardless of screen width.
Screenshots
Before
After
- Let's file an issue for those styling bugs (we don't seem to be aware of them).
In my opinion, I feel that quick fixes like this would not need an issue filed if a PR is opened. If an issue is required, however, I can file one.
2. Can we be more explicit in the PR description (Problem/Solution sections) as to what issues the PR fixes how, and what tweaks it performs (there are three distinct changes and it is not obvious to me what effects these have, so that context would be helpful)?
Apologies, I thought that the description and screenshots provided were explicit enough. The changes performed are actually not distinct and all for the same purpose.
3. Please make sure to follow conventional commits in yari. Technically, there is no need for the individual commits to follow it, as long as the PR title does, because it is used for the squash commit message. For example:
fix(sidebar): <concise description of the changes>
Sorry, I don't usually use conventional commit messages on any of the projects I work on, and I forget that Yari requires them. If I forget, please feel free to fix the title accordingly.
4. The media query condition you're removing seems to have been added in fix(sidebar): fix scrolling behavior #8512 to fix an issue, so we need to be sure your proposed changes don't cause any regression or any other undesired side-effect, especially regarding the sidebar placement (ad).
Looking at that PR, I see that one of the two issues that PR is intended to fix is the left navbar overflow. This actually happens to be the very same (or at least, extremely similar) issue that this PR fixes -- in other words, the bug was only partially fixed in #8512.
From what I can tell, the introduction of @media screen and not (min-height: $screen-height-place-limit) {
is a part of the issue as to why there is still a huge margin below the footer. This media query was introduced in #8512. Most likely, this fixed the rendering for the screen size specified (1400x700), but did not fix the rendering on a different viewport height at the same width.
I can confirm that the TOC (right) sidebar is not affected by these changes.
This breaks the side bar an small heights:
Before:
After:
Weird...I am not seeing that on my end on either Chrome or Firefox (getting script errors in Safari so I can't test there). What are the dimensions of the viewport, and what browser are you using?