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 multiple cdn #6982

Closed
wants to merge 9 commits into from
Closed

Add multiple cdn #6982

wants to merge 9 commits into from

Conversation

peng
Copy link
Contributor

@peng peng commented Feb 22, 2024

Description

Add build.assetsPrefix config docs if is object.

Related issues & labels (optional)

PR: withastro/astro#10189

Issues discussed: withastro/astro#9700 (comment)

Opened discussion: withastro/roadmap#818

Copy link

vercel bot commented Feb 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Feb 23, 2024 6:45pm

@github-actions github-actions bot added the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Feb 22, 2024
@astrobot-houston
Copy link
Contributor

Hello! Thank you for opening your first PR to Astro’s Docs! 🎉

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any broken links you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Vercel 🥳

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

Copy link
Contributor

@VoxelMC VoxelMC left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for submitting these wonderful Docs changes.
There are a few things to get out of the way first!

  1. I see that this documents a currently proposed change! Once that change gets through scrutiny, it will be merged when appropriate :) (minor release day!)
  2. In Astro Docs, we tend to write the en document first, merge it, then translate them. This way, it is less of a headache for the maintainers to check that every page is correct, and it's better for the translators to keep track of what is happening with the pages! You are more than welcome to PR the translations post-merge! I may be wrong about this; if I am, let me know (maintainers). Just letting you know before moving forward!
  3. We may need to go through some iterations to ensure the documentation is consistent with current writing styles! I, and I am sure others, would love to work with you to make that happen.

Here's looking at getting that Astro PR merged and then this one, subsequently!

src/content/docs/en/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
**If use `Record<string, string>`**
</p>

Applicable to multiple CDN scenarios. If you static file use different CDN, you can configure it like the example below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is documenting new behaviour, we will need to double check with core for accuracy!

src/content/docs/en/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/es/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@VoxelMC VoxelMC left a comment

Choose a reason for hiding this comment

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

This review comment is more for overall edits.
You have done a great job of writing out what this new change brings! Quite frankly, it's pretty cool :)
I have suggested some changes, but if you feel like they are incorrect, feel free to ignore them or discuss with me how we can meet somewhere in the middle!

Once more, thank you for working on this!!!

src/content/docs/en/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/configuration-reference.mdx Outdated Show resolved Hide resolved
@sarah11918
Copy link
Member

Hi @peng - Thank you for contributing!

This will take a little bit of work to resolve properly, because in fact this is not the appropriate place to make changes to the configuration reference file, nor should any changes be made to other languages.

You should see at the top of this page when you edit it to tell you which file holds the source for this documentation:
image

Because this is also a feature / code change, the proper PR for this documentation is the same one you made your code changes in. That PR should also edit the above file and include documentation there, in proper JSDoc formatting. So, it's a little trickier because it must follow those syntax rules. You should look at other entries on the page and follow the model of how they are written.

Also, no changes should be made to any translated files. I'm assuming you do not speak all of these languages? 😄 We do not use any automated translations in our docs, and we have a system for our human translators, who are native speakers, to handle translations after a change is made to the English documentation. Most documentation contributions do require editing, so it is also inefficient to try to provide all languages up front before we have edited the English content.

So, this PR will eventually be closed. But, we can use this PR for some first edits, then you will know what to add to the other Astro PR and we can polish final documentation edits in that PR!

@github-actions github-actions bot removed the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Feb 23, 2024
@sarah11918 sarah11918 added not for this repo This should be addressed in another repo (e.g. config ref docs, a bug in Astro etc.) add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) labels Feb 23, 2024
@sarah11918
Copy link
Member

sarah11918 commented Feb 23, 2024

FYI - I removed the changes to the other languages so that I could remove our automated i18n label -- it will keep adding itself back as long as there is a change to a translated page, and then it's harder for me to label properly for triage! 😄

Co-authored-by: voxel!() <voxelmc@hotmail.com>
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.

Just putting a block on this to make sure it doesn't get merged here. This content should move to the other linked Astro PR.

@peng
Copy link
Contributor Author

peng commented Mar 4, 2024

@sarah11918 OK, I will open another PR.

@peng peng closed this Mar 4, 2024
@peng peng mentioned this pull request Mar 4, 2024
@peng
Copy link
Contributor Author

peng commented Mar 4, 2024

@VoxelMC @sarah11918 I opened another PR: #7183. Only en docs. Create by astro/src/@types/astro.ts file.

@peng peng deleted the add-multiple-cdn branch March 13, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) not for this repo This should be addressed in another repo (e.g. config ref docs, a bug in Astro etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants