Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Feb 20, 2024

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

Screenshot 2024-02-20 at 06 56 50 Screenshot 2024-02-20 at 06 57 27

After

Screenshot 2024-02-20 at 06 55 37 Screenshot 2024-02-20 at 06 57 34

@queengooborg queengooborg requested a review from a team as a code owner February 20, 2024 15:02
Copy link
Contributor

@caugner caugner left a 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:

  1. Let's file an issue for those styling bugs (we don't seem to be aware of them).
  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)?
  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>
  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).

Thank you for your understanding. 🙏

@caugner caugner marked this pull request as draft February 28, 2024 13:03
@queengooborg
Copy link
Collaborator Author

queengooborg commented Mar 1, 2024

  1. 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.

@queengooborg queengooborg changed the title Update sidebar styling to fix margin-related issues fix(sidebar): Update sidebar styling to fix margin-related issues Mar 20, 2024
@queengooborg queengooborg marked this pull request as ready for review March 20, 2024 09:12
@fiji-flo
Copy link
Contributor

This breaks the side bar an small heights:

Before:

image

After:

image

@queengooborg
Copy link
Collaborator Author

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?

@github-actions github-actions bot added the idle label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants