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(docz): ordering menu on doczrc #322

Merged
merged 3 commits into from
Sep 12, 2018

Conversation

mpivaa
Copy link
Contributor

@mpivaa mpivaa commented Sep 11, 2018

Ordering menu on doczrc

Based on http://feedback.docz.site/roadmap/p/managing-menu-order-on-doczrc

Pre-merge checklist

  • Order menu through doczrc
  • Update docs

@mpivaa mpivaa changed the title feat(docz): ordering-menu-on-doczrc feat(docz): Ordering menu on doczrc Sep 11, 2018
@mpivaa mpivaa changed the title feat(docz): Ordering menu on doczrc feat(docz): ordering menu on doczrc Sep 11, 2018
@pedronauck
Copy link
Member

pedronauck commented Sep 11, 2018

Great job @mpivaa, you're doing a really great work, thanks 👏

So, about this task, sorry to tell that just now, but I was thinking of something a little bit different and maybe a little bit complicated too...

The idea is to use the menu property` to create the hierarchy and be able to add some custom properties for each one:

export default {
  menu: [
    'Getting Started',
    {
      name: 'Components',
      menu: [
        'Alerts',
        'Button',
        ...
      ],
    },
    {
      name: 'Github',
      route: 'https://github.com/pedronauck/docz'
    }
  ]
}

But this is can be kinda a breaking change because of today when we use <Docs> component inside a theme to get the array of menus, this array just has strings. And to keep maintainability, we should need to use the menu to ordering and populate the data.

if (a < b) return reverse ? 1 : -1
if (a > b) return reverse ? -1 : 1
return 0
}

const UNKNOWN_POS = 999999 // TODO: fix this?
Copy link
Member

Choose a reason for hiding this comment

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

I think that use Infinity like the sortOrder method in this link is better:
https://github.com/mdx-js/mdx/blob/master/docs/_app.js#L49-L60

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 about Infinity too, I will change

@mpivaa
Copy link
Contributor Author

mpivaa commented Sep 11, 2018

I think these changes makes sense, but definitely they are breaking changes, maybe we should always pass an object as the menu and the theme handles how to present it?

@pedronauck
Copy link
Member

Yeah, for sure @mpivaa, this will be a breaking change definitely 😕 So, I think that is best we keep baby steps here and release this feature like people want, in a simple way and. Maybe leave this change to release in v1.0 and we can put some others nice things together ✌️

@hnordt
Copy link

hnordt commented Sep 11, 2018

@pedronauck I can see you are not using menu in doczrc.js. Why you don't allow passing menu and keep the current behavior when it's omitted. It would prevent a breaking change.

@mpivaa
Copy link
Contributor Author

mpivaa commented Sep 11, 2018

@hnordt It would break if the user defines menu in doczrc.js but the theme is not prepared for an object.

@mpivaa
Copy link
Contributor Author

mpivaa commented Sep 11, 2018

@pedronauck I've changed the config interface so it looks like:

export default {
  menu: [
    'Getting Started',
    {
      name: 'Components',
      docs: [
        'Alerts',
        'Button',
        ...
      ],
    }
  ]
}

That way when we change the implementation of Menu to allow objects, it will not break old configurations.

@pedronauck pedronauck merged commit 0efb905 into doczjs:dev Sep 12, 2018
@mpivaa mpivaa deleted the order-menu-on-doczrc branch September 12, 2018 00:58
@actuallyReallyAlex
Copy link

If I were to try and implement this now, would it work? v0.11.2

@miketdonahue
Copy link
Contributor

What version will this be released on @pedronauck @mpivaa ?

@pedronauck
Copy link
Member

pedronauck commented Sep 25, 2018

Will be released in the v0.12.0 @sidewalksalsa, we have just some bugs and performance improvements to solve before release this version 🙏

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

Successfully merging this pull request may close these issues.

None yet

5 participants