search

facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
56.77k stars 8.55k forks source link

Docs layout issues when announcementBar visible #8370

Open slorber opened 1 year ago

slorber commented 1 year ago

Have you read the Contributing Guidelines on issues?

Prerequisites

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 https://github.com/facebook/docusaurus/issues/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

Self-service

r-lawrence commented 1 year ago

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 commented 1 year ago

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

Djunnni commented 1 year ago

i will try this issue with #8374 https://github.com/facebook/docusaurus/pull/8377#issuecomment-1351468040

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

arbitrarymahi commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@slorber Please check my PR for this issue: https://github.com/facebook/docusaurus/pull/8829

slorber commented 1 year ago

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.

r-lawrence commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

https://github.com/facebook/docusaurus/assets/62929526/f586258c-ac18-4749-b46a-6a967096c191

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

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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