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

feat(v2): doc navbar item type #3539

Merged
merged 6 commits into from
Oct 7, 2020
Merged

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Oct 5, 2020

Motivation

Ability to link to a doc from the navbar, and have the link have a doc-sidebar-active class whenever that doc's sidebar is active.

This makes it possible to have tab-like navigation, see ReactNative website:

image

Note, in v1 this feature existed already with a quite similar API:
image

fixed #3320

Note: should we use this on Docusaurus website?
I've done so but maybe I should revert.
We should think better about how to organize v2 docs.

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

Tests, preview and dogfooding

@slorber slorber requested a review from lex111 as a code owner October 5, 2020 16:21
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 5, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 5, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 8171d05

https://deploy-preview-3539--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator Author

slorber commented Oct 5, 2020

TODO:

  • need to fix mobile
  • need to fix older versions

@@ -96,3 +98,32 @@ html[data-theme='dark'] .DocSearch {
var(--ifm-color-emphasis-100) 100%
);
}

.navbar__items > .navbar__item {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code will shift to Infima repo, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it necessarily has to, but not sure yet.

It's a CSS/infima breaking change, and those changes are only mandatory for sites that are willing to provide a tab bottom border for selected items (like RN website),

But sites could as well decide that just highlighting the link related to a sidebar (without the bottom layout) is enough. In such case, the change to Infima could be minimal and look like;

    &:hover,
    &--active,
+   &--group-active {
      color: var(--ifm-navbar-link-hover-color);
      text-decoration: none;
    }

https://github.com/facebookincubator/infima/blob/master/packages/core/styles/components/navbar.css#L132

I could even just apply the existing --active modifier and not modify Infima as all, wonder what's best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My PR mostly focus on the behavior side of the navbar item (ie applying a className when any of the docs of the same sidebar is focused).

The styling side could be handled in a separate PR imho, just added this custom site CSS to demonstrate that the behavior is good enough to add this styling from the outside.

The point of this PR is not really to discuss how navbar links should look like between these 2 options:

image

image

Do you think I should revert the v2 custom styles (ie move back to simply coloring the navbar link? 2nd option) until we figure out how to make this through Infima

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could even just apply the existing --active modifier and not modify Infima as all, wonder what's best.

Oh, it's really easier, so maybe we choose this option?

@lex111
Copy link
Contributor

lex111 commented Oct 6, 2020 via email

@slorber
Copy link
Collaborator Author

slorber commented Oct 6, 2020

Yes that's a possibility.

Do you think we should enable the tab-like CSS for all docusaurus sites?

Should I just apply the --active modifier in case we are browsing a doc of the same sidebar?

v1 had 2 distinct classes, to distinguish between active doc vs inactive doc but a doc of same "group" (ie sidebar) is active:

image

What about a --group-active modifier?

@slorber
Copy link
Collaborator Author

slorber commented Oct 6, 2020

Going to revert my local CSS changes so that we can add this styling in a new PR if we want to. This way this PR would be minimal and only focus on the behavior and we can merge it asap.

Now I just highlight the element, no need for any CSS change:

image

As a backup here are the styles I used to get the tabs working, inspired by CSS on RN website:

:root {
  --ifm-navbar-padding-vertical: 0;
}

.navbar__items > .navbar__item {
  position: relative;
  height: 100%;
  display: flex;
  align-items: center;
}

.navbar__items > .navbar__item.navbar__link.navbar__link--active::after {
  content: '';
  position: absolute;
  bottom: 0;
  left: 0;
  width: 100%;
  height: 0.25rem;
  background-color: var(--ifm-link-color);
}
.navbar__items > .navbar__item.navbar__link.doc-sidebar-active::after {
  content: '';
  position: absolute;
  bottom: 0;
  left: 0;
  width: 100%;
  height: 0.25rem;
  background-color: var(--ifm-link-color);
}
.navbar__items > .navbar__item.navbar__link.doc-sidebar-active {
  color: var(--ifm-link-color);
}

@slorber slorber marked this pull request as ready for review October 6, 2020 13:41
@lex111
Copy link
Contributor

lex111 commented Oct 6, 2020

Do you think we should enable the tab-like CSS for all docusaurus sites?

Yes I think it's great!

Should I just apply the --active modifier in case we are browsing a doc of the same sidebar?
What about a --group-active modifier?

Is it really necessary? I don't really understand, but can we use only class with --active modifier?

@slorber
Copy link
Collaborator Author

slorber commented Oct 6, 2020

Is it really necessary? I don't really understand, but can we use only class with --active modifier?

Not sure, hard to predict, but as it's a new feature as long as people don't ask for it... Made the "group active" class configurable, with navbar__link--active as default, so we should be fine.

Leaving right now but if CI pass and it's ok for you, you can merge ;)

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 6, 2020
@lex111
Copy link
Contributor

lex111 commented Oct 6, 2020

The only thing that confuses me is that we have now two essentially duplicate links, and therefore two navbar items are highlighted, although there should be one.

image

I think we need to move versions dropdown from navbar to doc sidebar. Something like on the Vue docs:

image
https://vuejs.org/v2/guide/

Or we can follow the Kubernetes Docs path - not show the currently browsing version in the navbar, but just a label "Versions":

image
https://kubernetes.io/docs/concepts/overview/what-is-kubernetes/

@slorber
Copy link
Collaborator Author

slorber commented Oct 7, 2020

Yes that could be a nice idea to have the version dropdown in the sidebar, we should probably do a redesign pass of the Docusaurus UX at some point, including the mobile experience.

The duplicate highlighting is annoying that's true. However I don't think we can solve this in a generic way, as the user can compose the navbar items how he wants to, he can add 2 items from the same sidebar and end up by default with 2 highlighted items anyway (which is quite similar to what happens in this case btw).

I've added an option to make it more flexible, and so that our site is less confusing too:

        {
          type: 'docsVersionDropdown',
          position: 'left',
          dropdownActiveClassDisabled: true,
        },

Do you think it's good enough to get merged?

To me it's fine, as we don't force any user to use this new doc item type anyway

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lex111 lex111 merged commit 9ba28a3 into master Oct 7, 2020
@lex111 lex111 deleted the slorber/docs-sidebar-navbar-item branch October 7, 2020 12:16
@slorber
Copy link
Collaborator Author

slorber commented Oct 7, 2020

Thanks

About the K8 versions drropdown, it's indeed possible to make the label static and provided by option. Not sure if it's better but it's a possibility. However in the past many users asked for the current version to be visible in the navbar so not sure people would be interested by that VS the current dropdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need for DocNavbarItem (v1 feature)
4 participants