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

Docs layout issues when announcementBar visible #8370

Open
2 of 7 tasks
slorber opened this issue Nov 24, 2022 · 13 comments
Open
2 of 7 tasks

Docs layout issues when announcementBar visible #8370

slorber opened this issue Nov 24, 2022 · 13 comments
Labels
bug An error in the Docusaurus core causing instability or issues with its execution difficulty: advanced Issues that are complex, e.g. large scoping for long-term maintainability. help wanted Asking for outside help and/or contributions to this particular issue or PR.

Comments

@slorber
Copy link
Collaborator

slorber commented Nov 24, 2022

Have you read the Contributing Guidelines on issues?

Prerequisites

  • I'm using the latest version of Docusaurus.
  • I have tried the npm run clear or yarn clear command.
  • I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
  • I have tried creating a repro with https://new.docusaurus.io.
  • I have read the console error message carefully (if applicable).

Description

image

When the announcementBar is visible, the docs sidebar has a weird layout issue

Page: https://docusaurus.io/tests/docs

Good URLs to reproduce (long sidebar collapsed - long content):

Note: it's not because of the hideable sidebar feature, the issue is also there when there's no sidebar collapse button

Also related to #7815: the announcement bar height should probably be dynamic/flexible and layout should rather adapt gracefully to any size.

Reproducible demo

No response

Steps to reproduce

https://docusaurus.io/tests/docs

Expected behavior

No weird layout with elements overlapping

The visible scrollbar takes the appropriate height.

Actual behavior

Elements overlap and the scrollbar does not go up to the bottom (the scrollbar should touch the collapse button)

Your environment

  • Public source code:
  • Public site URL:
  • Docusaurus version used:
  • Environment name and version (e.g. Chrome 89, Node.js 16.4):
  • Operating system and version (e.g. Ubuntu 20.04.2 LTS):

Self-service

  • I'd be willing to fix this bug myself.
@slorber slorber added bug An error in the Docusaurus core causing instability or issues with its execution help wanted Asking for outside help and/or contributions to this particular issue or PR. good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. labels Nov 24, 2022
@r-lawrence
Copy link
Contributor

r-lawrence commented Nov 25, 2022

Hi everyone!
I have investigated this issue and believe I have created a fix in this PR. It appears that a CSS property of sticky is causing the overlapping issue seen in the above screenshot. I have removed the CSS property and was hoping to get some reviews. Thanks a bunch!

@msaini28r
Copy link

@slorber I would like to work on this issue if it is available.

@Djunnni
Copy link
Contributor

Djunnni commented Dec 23, 2022

i will try this issue with #8374
#8377 (comment)

If I'm too late, someone else can do it!

sbeaury pushed a commit to sbeaury/docusaurus that referenced this issue Dec 31, 2022
sbeaury added a commit to sbeaury/docusaurus that referenced this issue Jan 1, 2023
@arbitrarymahi
Copy link

Hello @slorber, I have found the solution to this problem, How do I make sure the fix doesn't create other issues, can you please help, it's my first time contributing.

@slorber
Copy link
Collaborator Author

slorber commented Mar 22, 2023

@arbitrarymahi you can create a pull-request and we'll review it.

Note: all attempts to fix this have failed so far, it's not an easy issue to take for a first contribution so make sure you tested all the edge cases.

@arbitrarymahi
Copy link

@slorber Please check my PR for this issue: #8829

@slorber slorber added difficulty: advanced Issues that are complex, e.g. large scoping for long-term maintainability. and removed good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. labels Apr 7, 2023
@slorber
Copy link
Collaborator Author

slorber commented Apr 7, 2023

Hi community

Thanks for all contributors that gave it a try to solve this annoying layout problem.

We received multiple PRs but no implementation provided was satisfying enough.

All the proposed PRs had bugs in the review URLs I suggested:

This problem is likely more complex to solve than it seems, and I'll probably have to handle it myself instead of asking for community help.

For now, I suggest that the community step back on this issue and stop submitting PRs. I'm going to do the cleanup and close the existing PR attempts.

If you still think you can solve it properly and submit a PR, please review carefully the 2 URLs above so that there is no issue at all (we are not looking for a partial fix here) on different viewport sizes with/without the announcement bar. I'll review these pages very carefully on your PR so doing this work upfront ensures that your PR is valid.

@slorber slorber changed the title Docs sidebar layout issue when announcementBar visible Docs layout issues when announcementBar visible Apr 7, 2023
@r-lawrence
Copy link
Contributor

r-lawrence commented May 10, 2023

Hi Everyone!

I took another stab at this issue. Initially I was only looking at CSS functionality which was the incorrect approach. When the announcement bar is shown, the root of the problem is we need to adjust the height dynamically for the entire sidebar. The details of everything are included in this PR.

Additionally I have one question regarding argos continuous integration check. It appears based on my changes, elements positions changed. Looking at the screenshots from the CI, the changes appear to be minor. I've never used argos before and I would like someone else to look over them to ensure I didn't miss anything, before I mark them as ok.

Thanks a bunch for taking the time to look this over and let me know if any other changes should be made!

@slorber
Copy link
Collaborator Author

slorber commented May 10, 2023

Initially I was only looking at CSS functionality which was the incorrect approach.

@r-lawrence will review your PR on time but please make sure your solution works BEFORE react hydration. If it's using hooks it's likely working only after hydration.

For me, the solution is rather CSS math + inlined JS, and I'll review carefully that it works as I expect it to. Please test your PR with slow 3G mode activated and ensure there are no unexpected layout shifts during the React hydration, and that it works fine if we attempt to scroll before the hydration.


Argos CI is new, I'll review the UI diff reported, but wonder why your PR should involve minor changes at all.

before I mark them as ok.

Are you able to do that? This is unexpected.

@r-lawrence
Copy link
Contributor

r-lawrence commented May 10, 2023

will review your PR on time but please make sure your solution works BEFORE react hydration. If it's using hooks it's likely working only after hydration.

I didn't have to much time to take a look at this right now, but I think you are correct. When running the functionality on 3G slow throttled mode in the chrome dev tools, there is a small layout shift. You can see it in this short video. The bottom sidebar ad and button are initially located incorrectly because the hydration has not occurred and the application has not detected the announcement bar. Then it adjusts its positioning, after the hooks events fire.

Screen.Recording.2023-05-10.at.10.31.52.AM.mp4
  • I think I may be a bit confused... How does the announcement bar show up before hydration? It appears it managed by the useContext hook listed above. Is there a way we can somehow mimic this functionality for the sidebar too? IE - allowing for the application to detect if the announcement bar is present before hydration and displaying the proper height for the sidebar. Then using the hook to correctly adjust height based on scroll movement?
    • do you have any suggestions on how this can be done with strict CSS math + inline JS? I'de be happy to give them a shot.

Also, originally the functionality that used the useAnnouncementBar function from packages/docusaurus-theme-common/src/contexts/announcementBar.tsx was housed in packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/Content/index.tsx. To me, it appeared this functionality was added in an effort to fix the sidebar issue when the announcement bar was present. This functionality is what causes the margin to be off in the sidebar when the announcement bar is present. In turn, this also affects the scrollbar not extending to the bottom of the sidebar. (see screenshots below)
Screenshot 2023-05-10 at 10 46 44 AM
Screenshot 2023-05-10 at 10 48 33 AM

  • Even if this solution isn't viable, should we still remove the functionality added to packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/Content/index.tsx to add margin when the announcement bar is detected? This is causing the bug thats related to the scrollbar not extending to the bottom of the sidebar ad.

Argos CI is new, I'll review the UI diff reported, but wonder why your PR should involve minor changes at all.

Looking at this quickly, some css was changed to allow for the sidebar ad and button to always be positioned at the bottom of the page. The resulting UI changes could be from some of this CSS. There was two places I changed css for height - packages/docusaurus-theme-classic/src/theme/DocRoot/Layout/Sidebar/styles.module.css and packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/styles.module.css. One of these changes could be causing the layout shift in Argos?

Thanks for your response and when I have additional time later, I'll be happy to continue looking at this for a bit!

@slorber
Copy link
Collaborator Author

slorber commented May 10, 2023

@r-lawrence I'm not super familiar with this code and can't help you much. It is there for a long time, was not coded by me.

I am not 100% sure of the solution to use and how many changes are required. If I help you figure things out, I would be faster to actually solve the problem myself.

What I'm sure of is the outcome we want: 0 layout shifts.
And for that, I believe reading localStorage in inlined JS + using CSS math could work. not sure but it's what I would try first.

If you are able to solve it on your own that's nice, otherwise, I can't really help and it's preferable that you close your PR.

@r-lawrence
Copy link
Contributor

@slorber Hey no problem. I'll work on this a bit later when I have some more time. If I can't come up with a solution in a reasonable time, I'll go ahead and close the PR. Thanks!

@ArchitGajjar
Copy link

Hi @slorber - is this issue still open ? I would like to give a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution difficulty: advanced Issues that are complex, e.g. large scoping for long-term maintainability. help wanted Asking for outside help and/or contributions to this particular issue or PR.
Projects
None yet
Development

No branches or pull requests

6 participants