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

Issue 305 broken link because of language fixes #322

Merged
merged 15 commits into from
Dec 20, 2017

Conversation

richardzcode
Copy link
Contributor

Motivation

Second attempt to fix #305

Fixed some bugs in the first PR. Tested many rounds in translation enabled/disabled cases.

Test Plan

  • website without translation.
    1. docusaurus-init
    2. npm run start
    3. check site
    4. npm run build
    5. cd build
    6. python -m SimpleHTTPServer 5000
    7. check site
  • website with translation.
    1. docusaurus-init
    2. npm run examples translations
    3. npm run start
    4. check site
    5. npm run build
    6. cd build
    7. python -m SimpleHTTPServer 5000

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@neilsutcliffe
Copy link
Contributor

Have you tested the GitHub versioning and page export? I have no idea how to test that bit myself.

@richardzcode
Copy link
Contributor Author

@neilsutcliffe I actually don't know what exactly are those. I haven't touched versioning part yet.

@richardzcode
Copy link
Contributor Author

Did a round test on Docusaurus site. Look good. Except one thing, after npm run build. If I run local python SimpleHTTPServer at build/ dir, assets would not load. I need to go into build/Docusaurus/ to run server. Is this expected?

@JoelMarcey
Copy link
Contributor

@neilsutcliffe I believe this PR, while touching versioning a little bit through some refactoring, does not affect versioning in any meaningful way. That said, I can run npm run version to test that out too. And by "page export", do you mean just building the static HTML assets so you can place them on any web server of choice? Like what is done via npm run build.

@JoelMarcey
Copy link
Contributor

@richardzcode I am testing this PR out now. Thanks!

@neilsutcliffe
Copy link
Contributor

I'm having a read up on Versioning now. It will be useful to know.

@neilsutcliffe
Copy link
Contributor

Well that would be testing deployment I guess. Can a docker container do that?

@JoelMarcey
Copy link
Contributor

@richardzcode I am running the Docusaurus site locally too. It is looking good. One thing your PR adds to our site is the translation dropdown.

screenshot 2017-12-19 13 52 44

vs. what is on docusaurus.io now.

screenshot 2017-12-19 13 53 31

I am initially thinking that this is a good thing since we do have translations enabled with a languages.js file. We just happen to support only one language at this time.

@JoelMarcey
Copy link
Contributor

@richardzcode

Except one thing, after npm run build. If I run local python SimpleHTTPServer at build/ dir, assets would not load. I need to go into build/Docusaurus/ to run server. Is this expected?

That's a good point. Yes, you need to go into the $projectName directory to run the site locally from the build files. That is admittedly not very clear in the docs. And begs the question whether we really need to output the $projectName when building to begin with.

@neilsutcliffe
Copy link
Contributor

I like it. Even if it only has one option. If the presence of a language file can determine if languages are available, then that is a win for convention!

@JoelMarcey
Copy link
Contributor

@richardzcode This is looking good. I have done testing on both the examples site from docusaurus-init or npm run examples as well as a local version of docusaurus.io, and things seem to be working fairly well.

There might be a few updates I make after we land this, but all in all, this should probably be our direction.

Thinking about this -- this could theoretically be a breaking change for some sites depending on how they have their index.js page implemented. But this may not affect most of our sites since they set the language to en anyway.

@richardzcode
Copy link
Contributor Author

Yeah I thought too showing dropdown was good even if only one language enabled. But it can be both ways. To keep consistent let's hide it.

@JoelMarcey
Copy link
Contributor

@richardzcode That's fair for now. But I might bring it back even for only one language :) I think I like the relationship of:

langauges.js exists => translations enabled => dropdown shown.

We could also have a config option called hideTranslationDropdown that forces the translation dropdown to be hidden. But I am not sure yet if that's a good idea or not.

@neilsutcliffe
Copy link
Contributor

neilsutcliffe commented Dec 19, 2017

There is also logic in headerNav that allows you to add a language box using the siteConfig.js but I'm guessing that should never be used?

else if (link.languages) {
      return (
        // return language dropdown
        <LanguageDropDown
          baseUrl={this.props.baseUrl}
          language={this.props.language}
          key="languagedropdown"
        />
      );

@JoelMarcey
Copy link
Contributor

@neilsutcliffe Hi.

There is also logic in headerNav that allows you to add a language box using the siteConfig.js but I'm guessing that should never be used?

links is based upon the headerLinks config in siteConfig.js with added information pushed to it.

e.g., in HeaderNav.js

renderResponsiveNav() {
    const headerLinks = this.props.config.headerLinks;
    // add language drop down to end if location not specified
    let languages = false;
    headerLinks.forEach(link => {
      if (link.languages) {
        languages = true;
      }
    });
    if (!languages) {
      headerLinks.push({languages: true});
    }

And the code you referenced is the only code where we render the LanguageDropDown.

So it is used all the time :)

@richardzcode could have, I suppose, put his checks there instead of directly in render() method of the LanguageDropDown component.

@richardzcode
Copy link
Contributor Author

right. the check is better to be outside. keep the logic of hiding dropdown for one language for now.

@neilsutcliffe
Copy link
Contributor

neilsutcliffe commented Dec 20, 2017

I just left a massive pull-request, which I don't expect to get approved for sometime, if at all that relates a bit to this issue.

#326

@JoelMarcey
Copy link
Contributor

@richardzcode I am going to try this merge again. :) I did testing on the test site and the Docusaurus site. All seemed pretty well, modulo some small changes that I will make afterwards.

@neilsutcliffe You many want to rebase #326 after this lands, assuming I don't have to revert it for whatever reason.

@JoelMarcey JoelMarcey merged commit a5e963d into facebook:master Dec 20, 2017
@JoelMarcey
Copy link
Contributor

Ok, here we go. :)

@JoelMarcey
Copy link
Contributor

https://docusaurus.io seems functional \o/

@JoelMarcey
Copy link
Contributor

JoelMarcey commented Dec 20, 2017

In this comment, I will list some auxiliary commits that I am going to make associated with this:

@richardzcode
Copy link
Contributor Author

Thank you @JoelMarcey !

For potential breaking of some sites. What I can think of is to create both default pages and 'en' pages.

In generate.js, after

    const docComp = (
      <DocsLayout metadata={metadata} language={language} config={siteConfig}>
        {rawContent}
      </DocsLayout>
    );
    const str = renderToStaticMarkup(docComp);

    let targetFile = join(buildDir, metadata.permalink);

add something like

    if (!env.translation.enabled) {
      // generate 'en' pages too
      targetFile = targetFile.replace('/docs/', '/docs/en/');
      writeFileAndCreateFolder(targetFile, str);
    }

But not sure if it is worth it. I personally would rather let user correct their index pages, since most of the users are just starting out.

@neilsutcliffe
Copy link
Contributor

I'm not sure about rebasing, but I merged the previous version of this PR (and will merge in the latest master too).

Obviously I had a lot of conflicts, but I'm in the right space to deal with them.

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.

Broken links in example index.js because of language
4 participants