fix: apply proper class for active doc item on mobiles + avoid duplicated classes #5264
Conversation
|
✔️ [V2] 🔨 Explore the source changes: 859271e 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61086c9d73984f0008f0533d 😎 Browse the preview: https://deploy-preview-5264--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5264--docusaurus-2.netlify.app/ |
|
Size Change: -119 kB (-13%) 👏 Total Size: 786 kB
ℹ️ View Unchanged
|
lex111
force-pushed
the
lex111/active-sidebar-class-fix
branch
from
a99925e
to
e8b897b
Compare
There was a problem hiding this comment.
activeSidebarClassName should be added if the user browser a doc that is inside the same sidebar as the navbar doc item.
This thing permits to enable a "tab-like" navbar item. In the following we can see that Component is highlighted when browsing the FlatList page, despite Components not directly linking to this FlatList page:
I agree that the duplicated className should be solved, however the point of having this extra class was to give the possibility for the user to disable this tab-like behavior. What if the user does not want to highlight the Components navbar item when browsing the FlatList page? What if we should highlight the Components navbar item in a different way for the Components index page and the FlatList page?
Forcing the existing Infima class means there's no way to opt-out of the tab-like styling behavior, which may not make sense in all cases.
As I'd like to introduce a navbar item of type docSidebar I think in the future we could even remove this tab-like behavior from item of type doc because the tab-like behavior would be a better fit for the docSidebar type?
As activeSidebarClassName is not used and is more complex than just a static className due to mobile, I agree we can remove it as a breaking change for now and see if anyone complains about this removal. If someone does, it's an opportunity to understand their use-case for it and figure out if a new docSidebar item type would solve it, or if we need to restore such an option.
| @@ -45,14 +45,19 @@ export default function DocNavbarItem({ | |||
| [activeVersion, preferredVersion, latestVersion].filter(Boolean), | |||
| ); | |||
| const doc = getDocInVersions(versions, docId); | |||
| const isCurrentDoc = activeDoc && activeDoc.sidebar === doc.sidebar; | |||
There was a problem hiding this comment.
Name isCurrentDoc is not really corrrect. I'd name this isDocSidebarActive instead
| const activeDocInfimaClassName = props.mobile | ||
| ? 'menu__link--active' | ||
| : 'navbar__link--active'; | ||
| props.activeClassName = activeDocInfimaClassName; |
There was a problem hiding this comment.
in this case, it's harmless but it's a bad practice to mutate React props. It would be better to create an extra variable
|
Agree, like the current navigation, and apparently so do our users. So let's remove |
slorber
added
the
pr: breaking change
|
👍 |
Breaking Changes
activeSidebarClassNameconfig, as it was probably never used by anyone and is confusing to use with Infima. Please let us know if you have a use-case for it, and we'll figure out a better api.Motivation
First of all, currently CSS class for active navbar item is duplicated, for example:
In addition to that, in mobile sidebar uses wrong
navbar__link--activeclass instead ofmenu__link--activefor active doc item.The duplication of CSS class occurs because option for adding custom class
activeSidebarClassNameis set by default innavbar__link--active. Although it's quite strange, because this option, as I understand it, is intended to add custom class to active doc item. So adding custom class viaactiveSidebarClassNameoption should not remove built-in Infima's active CSS classes.BTW I'm not sure this option
activeSidebarClassNameis needed at all at this moment. If you browse public repositories, you'll find that no one uses it -- default Infima class is set in all available repos. Well, maybe we should remove it?Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview.
On
/docspage: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.)