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

Allow subdirectories inside documentation folder #522

Closed
wants to merge 4 commits into from

Conversation

krakowski
Copy link

Motivation

This feature was requested in #323.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Since the HTML output is now stored within subdirectories as well, I had to adjust a glob pattern. After this change all tests ran successfully.

Both the development server and the build output (served using Python's SimpleHTTPServer) reflect the desired result.

Changes

  • A document's id is now determined by concatenating the relative path within docs and the document header's id field (or filename if the id field is missing).
  • Docusaurus documents are now organized in subdirectories.
  • The Navigation and Sidebars documentation has been updated.

Related Issues

Screenshots

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

@filkra Hi! Thank you for this PR. Sorry for the delay here - I am currently on vacation. I want to give this a real thorough review before approving/merging as this would be a very substantial change. I will look through this as I can while out, and then more when I get back.

Copy link

@vladikoff vladikoff left a comment

Choose a reason for hiding this comment

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

This is cool, I'm gonna try using this fork 😄

@@ -1,5 +1,6 @@
.DS_Store
.vscode
.idea

Choose a reason for hiding this comment

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

This should probably be globally ignored on your machine to avoid polluting .gitignore, but same goes for the other ignored dirs above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

"site-config"
"api/commands",
"api/doc-markdown",
"api/api-pages",

Choose a reason for hiding this comment

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

should this be just api/pages?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, yes you're right. I will change it later this day. Thanks!

config.baseUrl +
docsDir +
'/' +
language +

Choose a reason for hiding this comment

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

This might cause a bug here. If you compare this with the default website the /en/ is not part of urls.

But with this PR /en/ starts appearing in urls:

image

Maybe that should be determined by useEnglishUrl option

Copy link
Author

Choose a reason for hiding this comment

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

Hi, thanks for your feedback! By default website you mean docusaurus.io? If so, /en/ already appears to be a part of the url (https://docusaurus.io/docs/en/installation.html).

Choose a reason for hiding this comment

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

@filkra sorry, I meant the website you get after you run docusaurus-init

Copy link
Author

Choose a reason for hiding this comment

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

This seems to be more complicated than I thought... Using only useEnglishUrl doesn't work since the /en/ part will also be inserted, if a languages.js file exists. I ran docusaurus-init and created a languages.js file containing the following.

const languages = [
  {
    enabled: true,
    name: "English",
    tag: "en"
  }
];
module.exports = languages;

useEnglishUrl is set to false and the URL still contains /en/. Is this the right behaviour?

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.

@filkra Sorry for getting back to this so late. Could you rebase and update the branch? Will review this after that's done.

@@ -1,5 +1,6 @@
.DS_Store
.vscode
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

@yangshun
Copy link
Contributor

yangshun commented May 21, 2018

Closing due to inactivity. Feel free to reopen or submit a new PR if necessary.

@yangshun yangshun closed this May 21, 2018
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