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

fix: change subcategory format #1026

Merged
merged 5 commits into from
Oct 10, 2018
Merged

fix: change subcategory format #1026

merged 5 commits into from
Oct 10, 2018

Conversation

yangshun
Copy link
Contributor

@yangshun yangshun commented Oct 9, 2018

Motivation

The previous subcategory format had issues where the subcategory had to be an object. This means that all subcategories' siblings all have to be subcategories. In reality it should be possible to mix non-subcategories into the same layer.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Unit tests.

Example of subcategories in action:

screen shot 2018-10-09 at 12 33 37 am

Related PRs

#892

@yangshun yangshun requested a review from endiliey October 9, 2018 07:37
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 9, 2018
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Tests are failing. Had some suggestions after skimming through

"navigation",
"translation",
"versioning"
],
"Hello": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Its not in the suggested format ? It should be in array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I removed this but I didn't

v1/lib/server/readMetadata.js Show resolved Hide resolved
v1/lib/server/readMetadata.js Outdated Show resolved Hide resolved
id: subcategoryItem,
category,
subcategory,
sort: sidebarItems.length + 1,
Copy link
Contributor

@endiliey endiliey Oct 9, 2018

Choose a reason for hiding this comment

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

This is more of a question.

Do you think we need the sort. To be honest im not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm true. I'll remove this and see if it still works. This sort is added sequentially and it doesn't seem to do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need it because we make them into an object into readCategories, this sort field ensures that the order is not lost (not depending on the order of the keys, which is a good thing).

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 9, 2018

Deploy preview for docusaurus-preview ready!

Built with commit b6495d0

https://deploy-preview-1026--docusaurus-preview.netlify.com

@yangshun
Copy link
Contributor Author

yangshun commented Oct 9, 2018

@endiliey I basically rewrote the whole subcategory implementation because I just realized it only allows adding subcategories at the bottom of a category. I had to change the Blog metadata format also because it was using the same <SideNav /> component.

If you get the chance please patch and test it! 😄

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

I think in the future we might want to allow href linking on sidebars as well. #827

What do you think of changing the proposed format.

Sidebar item - subcategory

{
  type: 'subcategory',
  label: 'My Subcategory',
  ids: []
}

Sidebar item - url/href

{
  type: 'url'
  label: 'GitHub',
  href: 'github.com'
}

Sidebar item - doc id (default)

doc3

Eventually we have

'Second Category': [ 
  'doc3',
  {
    type: 'url',
    label: 'Github',
    href: 'github.com'
  },
  {
    'type': 'subcategory', 
   'label': 'my subcategory',
    'ids': ['doc4'],
  }, 
'doc5', 
],

This makes it scalable 😁. Imo

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Consider dogfooding one of our sidebars ?

@yangshun yangshun merged commit fe500de into master Oct 10, 2018
@endiliey endiliey deleted the subcategory branch October 11, 2018 12:32
@come-maiz
Copy link

Thank you @yangshun! I'm testing it and it's just what we need.
Well, almost... :) Is there a way to make the subcategory a drop down, so by default all the ids are hidden?

@yangshun
Copy link
Contributor Author

Yes, we'd have to add that feature in. Even if we add it in, the UX will not be ideal because the page does a full refresh and the expanded/collapsed state will be lost upon transition to new pages. Hence, I'm not that inclined on adding it. Any better ideas?

@come-maiz
Copy link

I wouldn't mind too much of getting it collapsed every time the page is opened. Remembering the state would be nice to have, but not a must IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants