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: collapsible categories #1128

Merged
merged 13 commits into from
Jan 23, 2019
Merged

feat: collapsible categories #1128

merged 13 commits into from
Jan 23, 2019

Conversation

tsmrachel
Copy link
Contributor

@tsmrachel tsmrachel commented Nov 25, 2018

Motivation

#1084

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tested against desktop browsers

  • Microsoft Edge 42.17134.1.0
  • Chrome Version 70.0.3538.110 (Official Build) (64-bit)

Test Plan

  1. Tested that it's working without collapsible menu enabled
  2. Added in subcategories and tested that both subcategories and links (under the category) is able to be collapsed/expanded
  3. Tested against full width and minimal width on desktop to ensure that UI looks fine
  4. Tested that the category remains expanded after navigating to a new page (link under subcategory) within the sidebar

image

image

Related PRs

Null

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 25, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 3814d17

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

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 25, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@endiliey endiliey requested a review from yangshun December 2, 2018 06:31
@endiliey
Copy link
Contributor

endiliey commented Dec 7, 2018

Mind to patch this PR with some dummy subcategory so we can preview it on https://deploy-preview-1128--docusaurus-preview.netlify.com/ ?

We will delete the test subcategory berfore merging (if approved)

Update: https://deploy-preview-1128--docusaurus-preview.netlify.com/docs/en/next/installation

@tsmrachel
Copy link
Contributor Author

@endiliey done! will need to remove/revert commit: ebf7ad8 if everything is fine

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks @tsmrachel! I tweaked the icon to use a chevron and the orientation (points to the right when collapsed, and points down when collapsed). This is more inline with typical chevron behavior. We really like this feature, many thanks!

@yangshun yangshun changed the title feat : #1084 Collapsus - The Collapsible Menu feat: collapsible categories Jan 23, 2019
@yangshun yangshun merged commit d5fd15e into facebook:master Jan 23, 2019
@@ -133,6 +146,29 @@ class SideNav extends React.Component {
<script
dangerouslySetInnerHTML={{
__html: `
var coll = document.getElementsByClassName('collapsible');
Copy link

Choose a reason for hiding this comment

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

Heyhey, is there any particular reason you're using var in favor of const/let?

Copy link
Contributor

Choose a reason for hiding this comment

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

The code within <script> is shipped as-is to browsers, and not all browsers understand let. We stick to ES5 in such situations.

Copy link

Choose a reason for hiding this comment

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

Thx for the clarification!

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.

7 participants