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

Add new github docs folder hosting option #520

Closed
wants to merge 4 commits into from
Closed

Add new github docs folder hosting option #520

wants to merge 4 commits into from

Conversation

andyeskridge
Copy link

This pull request fixes #501

The pull request adds instructions to the template readme and also adds a sequence of commands to the npm run build output.

image

I am happy to hear suggestions for making this better. I am wondering if the output after running npm run build is getting a little too long after adding this option. Maybe it would be good to settle on one option that we support for GH Pages?

@ghost
Copy link

ghost commented Aug 31, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@ghost ghost added the CLA Signed label Aug 31, 2016
@ghost
Copy link

ghost commented Aug 31, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Aug 31, 2016

I think we should go with only docs folder as the simpler option.

console.log(' ' + chalk.cyan('mv') + ' build/ docs/');
console.log(' ' + chalk.cyan('git') + ' add -f docs');
console.log(' ' + chalk.cyan('git') + ' commit -am ' + chalk.yellow('"Rebuild website"'));
console.log(' ' + chalk.cyan('git') + ' filter-branch -f --prune-empty --subdirectory-filter docs');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this command? I think it's only necessary for gh-pages because we needed to make a nested folder to be pushed as top-level.

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Left a few more comments, thanks again!

@andyeskridge
Copy link
Author

Thanks for the input @gaearon! I'll see if I can carve out some time this weekend to clean it up!

@ghost ghost added the CLA Signed label Sep 4, 2016
@andyeskridge
Copy link
Author

Sorry for the slow update. I've removed the old references and simplified the Github Pages documentation to focus only on deploying to a docs folder.

console.log(' ' + chalk.cyan('mv') + ' build/ docs/');
console.log(' ' + chalk.cyan('git') + ' add -f docs');
console.log(' ' + chalk.cyan('git') + ' add docs');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this need git add docs -A to remove old files from git? Could you please try to run these commands on a project and nail down the exact sequence before we recommend it?

@andyeskridge
Copy link
Author

https://andyeskridge.github.io/docsTest/ is the result of running the commands through twice.

The test was running through the commands after creating a clean create-react-app and then making a small change to the source and running through the commands again.

The -A for the git commit correctly removes the old files from version control now.

console.log(' ' + chalk.cyan('git') + ' add -f build');
console.log(' ' + chalk.cyan('rm') + ' -rf docs/');
console.log(' ' + chalk.cyan('mv') + ' build/ docs/');
console.log(' ' + chalk.cyan('git') + ' add docs -A');
console.log(' ' + chalk.cyan('git') + ' commit -am ' + chalk.yellow('"Rebuild website"'));
Copy link
Contributor

Choose a reason for hiding this comment

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

The -a flag is probably not needed because we added the docs in the previous command?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to keep for consistency so people don’t think it matters and stress over it.

console.log();
console.log(' ' + chalk.cyan('git') + ' commit -am ' + chalk.yellow('"Save local changes"'));
console.log(' ' + chalk.cyan('git') + ' checkout -B gh-pages');
console.log(' ' + chalk.cyan('git') + ' add -f build');
console.log(' ' + chalk.cyan('rm') + ' -rf docs/');
Copy link
Contributor

@fson fson Sep 16, 2016

Choose a reason for hiding this comment

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

If ./docs/CNAME file exists, we should preserve it. (Our current deployment code accidentally removes it, see #654.) When the custom domain is edited trough the GitHub Pages settings, GitHub saves it in this file.

git filter-branch -f --prune-empty --subdirectory-filter build
git push -f origin gh-pages
git checkout -
git push origin master
Copy link
Contributor

Choose a reason for hiding this comment

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

An extra space between push and origin.

@gaearon gaearon added this to the 0.4.2 milestone Sep 16, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

@fson
Thanks for reviewing!

@andyeskridge
Could you make these few changes? It’s good to go then. Cheers!

@gaearon
Copy link
Contributor

gaearon commented Sep 17, 2016

I tried to use this instead:

git commit -am 'Save local changes'
rm -rf docs/static # need to do that to not delete CNAME
mv build/* docs # the command in this PR moved build *into* docs, this is the correct one
git add docs -A
git commit -am 'Rebuild website'
git push

However even that won’t work on Windows because it doesn’t have mv and rm.
And I couldn’t figure out how to make Windows analogs work when e.g. docs doesn’t exist yet.

At this point I think this is the wrong approach.
We should instead print something like:

npm install -g deploy-to-github # I made up that name
deploy-to-github

If a package like this already exists we should use it.
Otherwise we should just create this package.
It will do the steps above in a cross-platform way.

Thanks everybody for discussion and sorry about long back and forth.

@gaearon gaearon closed this Sep 17, 2016
@gaearon gaearon removed this from the 0.4.2 milestone Sep 17, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change GitHub hosting instructions
4 participants