-
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): Update sidebar styling to fix margin-related issues #10560
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.
Thank you for taking the time to tackle these styling issues. 🙌
A couple of comments though:
- Let's file an issue for those styling bugs (we don't seem to be aware of them).
- 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)?
- 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>
- 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).
Thank you for your understanding. 🙏
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.
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.
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.
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 I can confirm that the TOC (right) sidebar is not affected by these changes. |
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? |
This pull request has merge conflicts that must be resolved before it can be merged. |
Still waiting for an answer to my previous question, CC @fiji-flo! |
#11621 This pr is also about this problem. |
FYI @queengooborg I just merged #11621, which fixes the overflow beyond the footer. Would you like to reduce your PR to adding the linear-gradient at the bottom of the sidebar on |
This pull request has merge conflicts that must be resolved before it can be merged. |
Since the critical issue was fixed through another PR, I've gone ahead and simply closed this PR. Thanks anyways! |
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