fix(v2): enable scrolling for sidebar menu only #2645
Conversation
|
Deploy preview for docusaurus-2 ready! Built with commit b377c23 |
|
FYI in Next.js docs has such a problem with sidebar menu height right now https://nextjs.org/docs/getting-started
I marked the empty bar with a red arrow - this issue solved in my solution, so there is so much code to fix that (we need to recalculate menu height after scroll of announcement bar). |
|
@yangshun any progress on this PR? |
There was a problem hiding this comment.
I'm not very keen on adding this given that:
- Not many people add announcement bar
- This only happens for people with a lot of docs and they have enough items open such that the page exceeds the viewport height
It's not like it's a very serious bug, the docs sidebar is still very usable.
The amount of code and complexity added to make this work far exceeds the benefit this fix brings :(
|
In fact, the main changes are regarding the addition of new context/provider for announcement bar, so we can combine this context with the context for the syncing tab choices feature. For example, it could be something like Yes, this is not a very serious bug, but this does not mean that we do not need to fix it. We cannot refuse to fix this issue just because the announcement bar is not such a popular feature. This fix applies not only to the announcement bar (I added support for this feature to avoid the problem as on Next.js docs). |
lex111
force-pushed
the
lex111/scroll-menu
branch
from
2564e25
to
b377c23
Compare
|
Fixed now. |
|
cc @yangshun |
| announcementBar: {id}, | ||
| }, | ||
| } = {}, | ||
| } = useDocusaurusContext(); |
There was a problem hiding this comment.
Subjective, but i'd rather use something like useDocusaurusContext().siteConfig.themeConfig?. announcementBar?.id
Motivation
This PR resolves #2620 and includes complex changes, mainly due to support for announcement bar. Since the height of the sidebar is based on the full height of viewport, when we are at the very top of the page, we see an announcement bar, which has a certain height, which should be taken into account when calculating the height of the sidebar.
In the screenshots below are full reflection of this challenge, for the solution of which we need to expose announcement bar info (to find out whether the announcement bar was dismissed or not) and limit the height of the announcement bar to a fixed height on the desktop.
Without announcement bar, sidebar menu scrolling is fully visible and everything is fine:
But if the announcement bar is shown, then bottom part of the sidebar menu scrolling is no longer visible, since we need to somehow take into account the height of this bar. I think it's not so easy to do.
To solve that, I created Provider+Context for the announcement bar, so that I could know from the DocSidebar component whether the bar was dismissed, and if not, then "adjust" the height of sidebar menu so that scrolling sidebar menu looks as expected.
As a result, we have the desired result! However how much code was needed to solve this!🤕
This PR contains two commits to show how much effort it took to get things to work with the enabled announcement bar.
Enough changes from the first commit 20bc7cc however we will have new issue with a scrolling of sidebar menu (as shown in the second screenshot).
We definitely do not want this, but I could not find an easier solution, maybe there will be thoughts on how to simplify this (@yangshun maybe I missed something?)
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)