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

Adds right-sidebar navigation made available in #475 to Blog Post pages #682

Merged
merged 3 commits into from
Jun 6, 2018

Conversation

ericnakagawa
Copy link
Contributor

@ericnakagawa ericnakagawa commented May 21, 2018

Recently #475 added an extra onPageNav navigation control to the right side of Docs pages.

This change adds the onPageNav sidebar nav from the DocsLayout pages to BlogsLayout pages.

Motivation

I think Blog posts could benefit form having a navigation control.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Enable onPageNav, run site, navigate to a blog post that uses headers H1, H2 and not it in right sidebar navigation.

Related PRs

#475

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 21, 2018
@ericnakagawa ericnakagawa requested a review from JoelMarcey May 21, 2018 23:14
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 21, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 4d178bf

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

@ericnakagawa
Copy link
Contributor Author

I don't think that this requires a docs edit. However, I can if you folks think it should be updated.

@@ -123,6 +123,11 @@ class BlogPostLayout extends React.Component {
</a>
</div>
</Container>
{this.props.config.onPageNav == 'separate' && (
<nav className="onPageNav">
<OnPageNav rawContent={this.props.children} />
Copy link
Contributor

@endiliey endiliey May 22, 2018

Choose a reason for hiding this comment

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

@ericnakagawa I think you missed importing OnPageNav ☺

const OnPageNav = require('./nav/OnPageNav.js');

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe @endiliey is correct. That's why the Circle test is failing.

I am confused though how it would work for you without the require in your testing 😮

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that’s embarrassing!

@joel I modify my local npm-module usually. I’ll push a fix.

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