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(5.0.0): nitpicking and suggestions regarding on-demand-rendering.mdx #9673

Conversation

ArmandPhilippot
Copy link
Contributor

Description (required)

  • Typo fix (every)
  • Updates some code snippet due to some concerns about highlighting (I only updated from the previous/next example but maybe all examples should use the same format: either simple highlighting or ins)
  • Updates the file name of a code snippet because the usage of Astro.params seemed wrong otherwise
  • Adds a suggestion regarding Astro.redirect in Response subsection

Related issues & labels (optional)

  • Suggested label: 5.0, 5.0.0-beta, code snippet update

@github-actions github-actions bot added the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Oct 14, 2024
Copy link

netlify bot commented Oct 14, 2024

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 686647d
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/670fbc910dcbb70008a5c6fa
😎 Deploy Preview https://deploy-preview-9673--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 configuration.

@ArmandPhilippot ArmandPhilippot changed the base branch from main to 5.0.0-beta October 14, 2024 21:58
@@ -38,7 +38,7 @@ All other pages are statically-generated at build time!

The following example shows opting out of prerendering in order to display a random number each time the endpoint is hit:

```js title="src/pages/randomnumber.js" {1}
```js title="src/pages/randomnumber.js" ins={1}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: The previous example use ins={2}, shouldn't we use the same visual information here? (or the other way around... not covered by my suggestion)


Then, if needed, you can choose to prerender any individual pages that do not require a server to execute, such as a privacy policy or about page.

```astro title="src/pages/about-my-app.astro"
```astro title="src/pages/about-my-app.astro" {2}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same concern here about the highlighting. The next example uses "highlighting" but we don't use it here. Maybe we should use ins here and in the next example too? (I just updated from the next example)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's make them all ins!

@astrobot-houston
Copy link
Contributor

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en guides/on-demand-rendering.mdx Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.


```astro title="src/pages/my-product.astro" {10,11}
```astro title="src/pages/product/[id].astro" {10,11}
Copy link
Contributor Author

@ArmandPhilippot ArmandPhilippot Oct 14, 2024

Choose a reason for hiding this comment

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

From my understanding, Astro.params only works with path segments (e.g. [id]). See Astro.params (besides on this page, I wonder if the example should not be above the sentence related to SSR...)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, where would you place this example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid any confusion I am talking about the example in the section Astro.params.
Sorry, I didn't detail as it's not related to this PR, it's just by checking Astro.params that I thought the location of the example seemed strange.
The example shows the use of getStaticPaths which is usable with static builds while the sentence just above talks about SSR builds. So what would seem more logical to me would be between the two sentences (In static builds,... > getStaticPaths example > In SSR builds, ...).
But thinking back, I guess that was to avoid isolating the "SSR builds" sentence.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha now, thanks! Yes, I see what you mean. I think it could be helpful on that page to also have an SSR example maybe? And then yes, the example there above the SSR sentence, which itself gets its own example?

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 almost forgot about this comment, sorry. Yes I think it would be useful to show both uses here. 💯 I'm going to prepare a PR.

@@ -211,7 +211,7 @@ You can also return a [Response](https://developer.mozilla.org/en-US/docs/Web/AP

The example below returns a 404 on a dynamic page after looking up an ID in the database:

```astro title="src/pages/[id].astro" {10-13}
```astro title="src/pages/product/[id].astro" {10-13}
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 only updated because that's the path I had used before... it wasn't really a problem, just a matter of consistency.

@github-actions github-actions bot removed the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Oct 15, 2024
@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. 5.0.0-beta labels Oct 15, 2024
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.

Thank you @ArmandPhilippot these changes look great to me! I made one ins suggestion where I could, and pointed out the one where I couldn't. But happy with this!

I will say, for the changes to the "Features" sections, which is basically copied from the existing server-side-rendering page in main branch, you can feel free to PR them to that page directly too, and this will let the translators work on that updated content in advance of this branch merging eventually. That would make that merge more just "copy paste" of that entire Features section, which I really hadn't intended to change here!

@ArmandPhilippot
Copy link
Contributor Author

Thanks for your feedback @sarah11918! I'm going to make a PR to update the current doc! 🫡

@sarah11918
Copy link
Member

!coauthor

@sarah11918 sarah11918 merged commit 5985401 into withastro:5.0.0-beta Oct 16, 2024
1 check passed
@ArmandPhilippot ArmandPhilippot deleted the guides/on-demand-rendering-suggestions branch October 16, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0.0-beta add new content Document something that is not in docs. May require testing, confirmation, or affect other pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants