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

Website: fix version display #582

Merged
merged 10 commits into from
May 8, 2018
Merged

Website: fix version display #582

merged 10 commits into from
May 8, 2018

Conversation

DanielMSchmidt
Copy link
Contributor

This PR has two different aspects, one is "releasing" the 7.X version of the docs.
The other one is having a versions page so that when you click on the current version on the top left. Here is a video of this:

detox-website-versions

@rotemmiz
Copy link
Member

rotemmiz commented Mar 4, 2018

I want to discuss doc duplication with you. I feel uncomfortable duping all docs 3 times (and potentially even more in the future). can't we generate the docs per version via git tags only on gh-pages branch during build ?

@DanielMSchmidt DanielMSchmidt force-pushed the website/versions branch 3 times, most recently from 517b7ff to 76e8698 Compare March 5, 2018 22:07
@@ -52,6 +52,12 @@ matrix:

# Website
- language: node_js
addons:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed by nodegit

@DanielMSchmidt
Copy link
Contributor Author

@rotemmiz I thought you were not sure about storing them there, I didn't understand earlier that this is really a problem for you.

The way I solved it now is by getting every git tag above version 6 (where we changed the docs to support docusaurus) and generate the version for each doc in a temporary copy of this repo, which is then copied over.

This is currently WIP, the sidebars don't work in the current implementation, need to figure out what's going wrong there 🤔

Is this the general direction you want to go with this or did I misunderstand you? 😇

@DanielMSchmidt DanielMSchmidt changed the title Website: fix version display [WIP] Website: fix version display Mar 7, 2018
@DanielMSchmidt DanielMSchmidt force-pushed the website/versions branch 2 times, most recently from 74e6108 to 584faa3 Compare March 8, 2018 19:38
@DanielMSchmidt DanielMSchmidt force-pushed the website/versions branch 2 times, most recently from 5e3ff4d to 1df93a1 Compare April 13, 2018 18:31
@DanielMSchmidt DanielMSchmidt changed the title [WIP] Website: fix version display Website: fix version display Apr 13, 2018
@DanielMSchmidt
Copy link
Contributor Author

@rotemmiz This is done now / ready for review.

I would like to move the code upstream to docusaurus, I think it is a better place for it. Easier to test and be concise this way (changing docusaurus versions could break this current workflow if they change something severe in the way they deal with versions). This is the issue I created for it: facebook/docusaurus#557

});
}

function getMajorVersion(tag) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use 'semver' instead of parsing versions manually, this will break the pretty easily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed that @rotemmiz 👍 Please review again!

@DanielMSchmidt DanielMSchmidt self-assigned this Apr 25, 2018
@DanielMSchmidt
Copy link
Contributor Author

@rotemmiz This is ready now 👍 I only clone the repo once and execute every action on this copy now :)

Copy link
Member

@rotemmiz rotemmiz left a comment

Choose a reason for hiding this comment

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

Great PR!
Have very few small notes.

await repo.checkoutBranch(version);
}

function applyTemporalFix(tempDir) {
Copy link
Member

Choose a reason for hiding this comment

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

applyMissingDocHeadersForV6 or whatever version which needs it.

@@ -0,0 +1,90 @@
/**
* Copyright (c) 2017-present, Facebook, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Is this Facebook's code ?
If not, remove the license note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's generated by docusaurus, should I remove this part?


function VersionLinks({version, isLatest = false}) {
const expandedVersion = version.replace('.X', '.0.0')
const isMaster = version === "master";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get what the final result will be. Will site show exact version (i.e. 7.3.5) or 7.0.0 throughout links ?

Copy link
Member

Choose a reason for hiding this comment

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

Video shows 7.X, 6.X and master. what am I missing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Video is old, but you can run it locally. Just do cd website/ && npm install && npm run gatherDocs && npm start and you can see the site

@@ -1,110 +0,0 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

<3<3<3<3<3<3<3<3<3<3<3<3<3<3

@DanielMSchmidt
Copy link
Contributor Author

Damn it, the checkout seems to have failed, unnoticed by me. Need to do some more node-git magic here.

@DanielMSchmidt
Copy link
Contributor Author

@rotemmiz I think I got your review comments, please take a look 👍

}

// https://stackoverflow.com/questions/29569913/switch-branch-tag-with-nodegit?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa
function checkOutTag(repo, tag) {
Copy link
Member

Choose a reason for hiding this comment

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

LOL, utm_source will provide false data for trackers next time someone opens this link ;p
JK, but the query string can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll get rid of that and see if I can get a better permalink

@DanielMSchmidt
Copy link
Contributor Author

@rotemmiz Got a spare review?

@rotemmiz rotemmiz merged commit 6cf3963 into master May 8, 2018
@wix wix locked and limited conversation to collaborators Jul 23, 2018
@LeoNatan LeoNatan deleted the website/versions branch July 22, 2019 02:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants