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

Migrate Website to Docusaurus 2 #1190

Closed
wants to merge 3 commits into from
Closed

Conversation

smorimoto
Copy link
Contributor

Context: #1171

@jvillard
Copy link
Contributor

Very cool, many thanks!

Is https://build-czv4ilhfc.now.sh/ an up-to-date preview of this PR?

As you mentioned on #1171, some content is missing from the landing page. I think we still want it there:

  • The asciinema animation "Infer in Action"
  • The Coderboard iframe "Try infer"
  • The "Who Uses Infer?" section

@smorimoto
Copy link
Contributor Author

Is https://build-czv4ilhfc.now.sh/ an up-to-date preview of this PR?

Yes, that's the latest preview.

As you mentioned on #1171, some content is missing from the landing page. I think we still want it there:

The asciinema animation "Infer in Action"
The Coderboard iframe "Try infer"
The "Who Uses Infer?" section

I'm working on it!

@smorimoto
Copy link
Contributor Author

smorimoto commented Nov 22, 2019

@jvillard Latest preview here!
https://build-8sx47qcn4.now.sh

@smorimoto smorimoto force-pushed the website branch 2 times, most recently from a8a5403 to 3405ab8 Compare November 24, 2019 13:36
@yangshun
Copy link
Contributor

@jvillard how does this look?

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.

Code looks good to me. Please add the copyright headers and we should be good to merge.

@smorimoto smorimoto force-pushed the website branch 3 times, most recently from 4ecd7ca to e4045e6 Compare November 27, 2019 10:51
@smorimoto
Copy link
Contributor Author

Rewrote tex section with MDX.

@smorimoto
Copy link
Contributor Author

@smorimoto
Copy link
Contributor Author

Any updates?

Copy link
Contributor

@jvillard jvillard 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 amazing, thank you! I think the website looks much better overall in the new version.

There are a few regressions compared to the current version, but I have no idea how hard they would be to address.

  • Is there a way to check for dead links? I spotted a few myself but it would be good to be somewhat confident that the migration didn't introduce dead links.
  • I cannot find the documentation menu when I reduce the window size. In the previous version you can click to expand the navigation menu, eg to jump directly to "Analyzing apps or projects" under the "User Guide" heading. In the new version I can only access the headings themselves directly using the links in the global footer.
  • The text in blog posts is smaller than it used to be, I'd prefer a larger font size if possible. Actually, the font size looks smaller on the whole site.
  • I've listed some more in inline comments
  • Do you have a list of features you weren't able to migrate and of textual changes made? This is the stuff I'm trying to track down in this code review but there's always a chance I missed some and it would be good to have at least a list of features that need to be re-evaluated (re-implement or leave dead), and a list of textual changes would make reviewing easier (mostly it would be nice if there were no textual changes at all in this PR and they were split in a separate one to make reviewing easier).

(note: I haven't reviewed all the files yet.)

@smorimoto
Copy link
Contributor Author

@yangshun I just deleted and node_modules and installed packages again, but somehow I get this error.

./node_modules/@docusaurus/core/lib/client/clientEntry.js
Error: Cannot find module './src/data'

@smorimoto
Copy link
Contributor Author

@jvillard I fixed problems. I will push latest version when build issue is fixed.

@smorimoto smorimoto force-pushed the website branch 2 times, most recently from 615fb21 to c5f622a Compare December 11, 2019 16:54
@smorimoto
Copy link
Contributor Author

@jvillard Published latest preview. (Sorry for late response...!)
https://build-cftwqp896.now.sh

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jvillard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@imbsky has updated the pull request. Re-import the pull request

Copy link
Contributor

@jvillard jvillard left a comment

Choose a reason for hiding this comment

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

Sorry for taking forever to review, end of year + holidays got in the way.

I think we are almost ready to merge, thanks so much for your work! Noted a few more things inline, although now it looks like I should be able to fix them myself after the fact if you prefer.

@smorimoto
Copy link
Contributor Author

@jvillard What do you use to deploy a static site? If you want to continue using GitHub Pages, I can set up a GitHub Action for deployment.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jvillard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jvillard
Copy link
Contributor

@imbsky keeping gh-pages sounds good to me. A GitHub action to automatically publish the website would be awesome!

@smorimoto
Copy link
Contributor Author

I'm going to create a sandbox repository and see if my idea really works!

@facebook-github-bot
Copy link
Contributor

@imbsky has updated the pull request. Re-import the pull request

@smorimoto
Copy link
Contributor Author

@jvillard This worked well in my environment. When merged into a master, it's automatically deployed to GitHub Pages.

@smorimoto
Copy link
Contributor Author

All environment variables are pre-configured in the Actions runner, so no extra configuration is required.

@facebook-github-bot
Copy link
Contributor

@imbsky has updated the pull request. Re-import the pull request

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jvillard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@smorimoto
Copy link
Contributor Author

Is there anything blocking merge?

@jvillard
Copy link
Contributor

I was just about to write back, sorry for the delay. I'm doing the last checks that everything looks good. There are a few adjustments to make on our side due to URLs to issue types being different now. I also had to fix a few minor things like dead links and syncing with the current website changes that happened in the meantime. Other than that it looks great! I'm hoping to land this today.

@jvillard
Copy link
Contributor

I'm unlikely to close this today so it will have to wait a few more days while I'm away. Again, sorry it's taking so long, we're almost ready on our end now.

@smorimoto
Copy link
Contributor Author

No worries! This amount of change is really annoying. I should have split it into a some small commits to make it easier to understand. I'm really sorry.

@smorimoto smorimoto deleted the website branch March 4, 2020 13:21
@jvillard
Copy link
Contributor

jvillard commented Mar 4, 2020

Merged! Many thanks again for your amazing contribution. The website is not pushed to fbinfer.com yet because we need to switch the config from jekyll to static pages, but should be done soon. The GitHub action worked perfectly, thanks for that too! :)

@yangshun
Copy link
Contributor

yangshun commented Mar 4, 2020

Hey @jvillard what does it mean by "to switch the config from jekyll to static pages"? Jekyll sites are static sites I believe.

@yangshun
Copy link
Contributor

yangshun commented Mar 4, 2020

I think I can try to help you deploy the liv site. It should just be a matter of running the deploy command and pushing to the gh-pages branch. Thanks so much @imbsky

@jvillard
Copy link
Contributor

jvillard commented Mar 4, 2020

@yangshun The new website was already pushed to gh-pages by the GitHub action but that broke fbinfer.com (only showed a generic error "There is no website there" or somesuch from GitHub). I reverted the push manually (see gh-pages' history).

Isn't the jekyll website generated on the fly by GitHub's infra from the markdown in the gh-pages branch? There's some setting to switch in our repo's configuration but I'm not an admin.

@yangshun
Copy link
Contributor

yangshun commented Mar 4, 2020

Oh I see. I think GitHub has special handling for Jekyll sites such that no configuration is needed. However when we moved to Docusaurus we will need to manually enable the gh-pages. @JoelMarcey could you help out here?

@jvillard
Copy link
Contributor

jvillard commented Mar 4, 2020

I created a task internally by the way: T63456491

@jvillard
Copy link
Contributor

jvillard commented Mar 4, 2020

Also it's very likely that I am completely wrong about how jekyll works!

@smorimoto
Copy link
Contributor Author

@smorimoto
Copy link
Contributor Author

No, it was not correct. Hmm... Why doesn't it work...

@smorimoto
Copy link
Contributor Author

Probably there is something wrong with the Docusaurus config file.

@yangshun
Copy link
Contributor

yangshun commented Mar 4, 2020

We need to manually allow gh-pages hosting from the repo settings, but only the Open Source team has access to that.

@jvillard
Copy link
Contributor

jvillard commented Mar 4, 2020

It's working now thanks to 79bbf57.

Many thanks!!! 🎉 🎉

@smorimoto
Copy link
Contributor Author

Yay!!

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

Successfully merging this pull request may close these issues.

4 participants