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

fix github pages deploy guide #1388

Merged
merged 17 commits into from
Aug 27, 2022
Merged

fix github pages deploy guide #1388

merged 17 commits into from
Aug 27, 2022

Conversation

kevinzunigacuellar
Copy link
Sponsor Member

@kevinzunigacuellar kevinzunigacuellar commented Aug 23, 2022

What kind of changes does this PR include?

  • New or updated content

Description (demo)

  • Closes Flesh out the GitHub Pages deployment guide #1355 and Branch is not allowed to deploy to github-pages #1376

  • Reorder steps: Pushing and committing the workflow before setting up pages in the repository throws an error

    Failed to create deployment (status: 404) with build version 
    fcb3c6ad752990f1ea041c7971c0c3f71ac43117. Ensure GitHub Pages has been enabled.
  • Switch Source from branch to Github Actions: deploying from branch uses Jekyll to deploy (error link)

  • Add a caution aside for people that added base only to deploy to GitHub pages to update their routes relative to base.

  • Added a fancy codeblock title

@netlify
Copy link

netlify bot commented Aug 23, 2022

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2cdcaa2
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/630961b6947197000861a36b
😎 Deploy Preview https://deploy-preview-1388--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@fredoist fredoist left a comment

Choose a reason for hiding this comment

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

Good work! 🔥

@swift502
Copy link
Contributor

swift502 commented Aug 24, 2022

These changes are great so far! Though maybe I'm alone, but I still don't understand this sentence:

If your repository is named <YOUR_USERNAME>.github.io, you don’t need to include base.

My website is named "jblaha.art". My repo is called "Web". So I thought I had to include the base parameter. But I didn't. It works without it. Actually, it's much better without it, because I don't have to update all my routes.

What is the purpose of this note?

@sarah11918
Copy link
Member

Hey @kevinzunigacuellar! Thanks for this! As you know, Chris and I are only checking in periodically on PRs this week, so things are moving a little slower here.

I just wanted to make sure you noticed that there are two other PRs/Issues (can't exactly remember!) about this very page... You probably already did, but since I'm not able to look closely right now, can you please look for them and try to amalgamate all the various bits people have called out or suggested as helpful for this page? Maybe tag those community members specifically in here so we can omg finally stop working on this page!! (Just kidding... I love all my Docs pages equally....) 💜

@kevinzunigacuellar
Copy link
Sponsor Member Author

These changes are great so far! Though maybe I'm alone, but I still don't understand this sentence:

If your repository is named <YOUR_USERNAME>.github.io, you don’t need to include base.

My website is named "jblaha.art". My repo is called "Web". So I thought I had to include the base parameter. But I didn't. It works without it. Actually, it's much better without it, because I don't have to update all my routes.

What is the purpose of this note?

I think this configuration is for when the name of your repo is exactly the same as your account. Yeah I didn't get it right of the bat as well, It could use a rewording.

@swift502
Copy link
Contributor

swift502 commented Aug 24, 2022

@kevinzunigacuellar even then I don't understand how the base would affect the deployment. In my case the base parameter just offset static assets by the base path, but it ultimately seemed to have no effect on how the project was deployed.

I'm definitely in favor of rewording. (If we can find someone who knows what it's trying to convey 😄)

@kevinzunigacuellar
Copy link
Sponsor Member Author

Base is meant exactly for that, to shift the built files by a base path. It's very convenient for github pages since they have a per repo deployment.

@swift502
Copy link
Contributor

swift502 commented Aug 24, 2022

@kevinzunigacuellar Could we then come up with an example of when it is useful, and put it on the page to make it clear?

And also, should it be done by default? As far as I can tell, if you don't already need a base path, you don't need to add one just so you can deploy. (It works for me that way) But the guide currently states it should be there by default, unless you have the specific case of a repository that shares the name with your username.

@kevinzunigacuellar
Copy link
Sponsor Member Author

kevinzunigacuellar commented Aug 24, 2022

I think it's possible to skip the base path to deploy but it can break other deployments if the user has multiple matching paths. In my opinion, we should try to follow Github's defaults and try to have all the page assets under the repository's name (e.g bob.github.io/repo-1 will contain all assets under /repo-1 and bob.github.io/repo-2 will contain all assets under /repo-2) so we may avoid some conflicts.

The note is there for the people that want to deploy in their root domain username.github.io using their matching username repo.

@sarah11918
Copy link
Member

@kevinzunigacuellar Let me know when you all are happy with this, then @Yan-Thomas or I will take a quick look over it and can merge!

@kevinzunigacuellar
Copy link
Sponsor Member Author

@kevinzunigacuellar Let me know when you all are happy with this, then @Yan-Thomas or I will take a quick look over it and can merge!

we are ready for review 😇

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Amazing job @kevinzunigacuellar, I just have a small nitpick but after this LGTM! Love to see this many contributors in a PR 💜

src/pages/en/guides/deploy/github.md Outdated Show resolved Hide resolved
@yanthomasdev yanthomasdev added the improve documentation Enhance existing documentation (e.g. add an example, improve description) label Aug 26, 2022
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Great work, everyone! 🥳 Not gonna lie, this page has caused me a lot of pain and suffering for a LONG time... thank you for jumping in and working so carefully to get it right!

I left some small comments! See what you think about them, and ping me when you've decided!

src/pages/en/guides/deploy/github.md Outdated Show resolved Hide resolved
@@ -15,15 +15,27 @@ Astro maintains the official `withastro/action` to deploy your project with very

1. Set the [`site`](/en/reference/configuration-reference/#site) and, if needed, [`base`](/en/reference/configuration-reference/#base) options in `astro.config.mjs`.
- `site` should be something like `https://<YOUR_USERNAME>.github.io`
- `base` should be your repository’s name starting with a forward slash, for example `/my-repo`.
- `base` should be your repository’s name starting with a forward slash, for example `/my-repo`. This is so that Astro understands your website's root is `/my-repo`, rather than the default `/`.

Copy link
Member

Choose a reason for hiding this comment

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

Is it too much to show an astro.config.mjs example here with those fields filled out?

Copy link
Contributor

@swift502 swift502 Aug 26, 2022

Choose a reason for hiding this comment

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

@odwrotnie was asking for an example in #1376. They managed to misplace the parameters outside of the defineConfig object, because they didn't find this clear enough. So perhaps it's a good idea.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I like the astro.config.mjs example here. Other deployment options with SSR show diff code blocks for configuring options even when most of this configuration is done by the CLI 😅.

src/pages/en/guides/deploy/github.md Outdated Show resolved Hide resolved
kevinzunigacuellar and others added 2 commits August 26, 2022 09:03
Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Kevin Zuniga Cuellar <46791833+kevinzunigacuellar@users.noreply.github.com>
@sarah11918
Copy link
Member

@kevinzunigacuellar @swift502 I took the liberty of throwing in an astro.config.mjs example. Please check and see what you think! (It only took me 5 commits, so... 😅 )

sarah11918 and others added 2 commits August 26, 2022 21:13
Co-authored-by: Kevin Zuniga Cuellar <46791833+kevinzunigacuellar@users.noreply.github.com>
@sarah11918
Copy link
Member

It's Friday night, and I think this is good to go! If we spot anything else later, let's just make a new PR. 😅

Thanks SO MUCH y'all! @kevinzunigacuellar @swift502 @fredoist @Yan-Thomas! GO TEAM! 🥳

@sarah11918 sarah11918 merged commit 7e4ffee into withastro:main Aug 27, 2022
@kevinzunigacuellar kevinzunigacuellar deleted the fix-github-pages-config branch August 27, 2022 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve documentation Enhance existing documentation (e.g. add an example, improve description)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flesh out the GitHub Pages deployment guide
5 participants