-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Runtime API restructure #10062
Runtime API restructure #10062
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have quickly looked over the page (the first half more in depth).
I think moving all headings up one step is a good decision cause before everthing was kinda nested under Astro global
which didn't provide much context. Generally speaking, it just looks cleaner...
I'm wondering if the h4 headings should be visible in ToC, maybe on this page it would be more beneficial to override it, but I also love if it is consistent across all pages, so we prob should just leave it as it is...
From the content point of view, I think I know the Astro basics too bad to comment on that. Let me read the docs first xD
Co-authored-by: trueberryless <99918022+trueberryless@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these add text highlighting, but there's a couple points relating to accuracy/missing content.
|
||
See also: [`Astro.locals`](#astrolocals) | ||
The locale computed from the current URL, using the syntax specified in your `locales` configuration. If the URL does not contain a `/[locale]/` prefix, then the value will default to `i18n.defaultLocale`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is undefined
if you have routing.prefixDefaultLocale
set to true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
Co-authored-by: Reuben Tier <64310361+TheOtterlord@users.noreply.github.com>
Co-authored-by: Reuben Tier <64310361+TheOtterlord@users.noreply.github.com>
Co-authored-by: Reuben Tier <64310361+TheOtterlord@users.noreply.github.com>
This reverts commit 43d1364.
Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
<button type="submit">Log out</button> | ||
</form> | ||
{result?.error && <p>Failed to log out. Please try again.</p>} | ||
```astro title="src/pages/index.astro" "Astro.site" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging that I had asked for an example here, and Reuben provided a placeholder one. Will this work, or do we need to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example is fine!
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
I could only find one entry that used multiple tabs ( |
I think Chris meant across the whole page. So if I select I think this is a good idea! I didn't add suggestions to the relevant places because, in that case, the key name ( |
<p> | ||
|
||
**Type:** `boolean`<br /> | ||
**Default:** `true` <br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already approved, but with Chris suggestions (🙌🏽 ) the content has changed a bit. Everything looks fine with a quick read (I didn't check for typo), but I'm a bit concern about this Default:
.
Both Per-page override
and Switch to server mode
are under this h2
. And while true
is the default in static mode, this is not the case (from my understanding) with server
mode. So it could be confusing.
I think we should either move Default:
into subsections or use a slightly more complex Default:
. Perhaps something like:
**Default:** `true` <br /> | |
**Default:** `true` with `output: 'static'` or `false` with `output: 'server'` <br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for commenting on this! I had wondered about this, because you're right, it's not the default in server
mode. It is true that it's default until you do something to change the nature of your project, which is how the text was handling it. I wasn't sure whether we had a standard way of either/or 'ing it, or whether it was in fact actually helpful to present the default as true
(because it is until something else happens.)
The wording kind of explains setting output: 'server'
as the way to change the default and I do like that framing. But I'm open to people's opinions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hesitant about that. I understood the intention given the structure of the page, but I am now quite familiar with how it works (I think 😄 ). So I totally understand what you mean.
But for someone new it might be less obvious... especially if they quickly scan the page instead of reading the explanations in detail...
(When I see some questions asked, sometimes I just want to say "but have you read the doc"!? I admire your patience! 🙄) So I don't know what the best way to solve this is, but I think it could be problematic.
Regarding the standard way of doing it, I don't think so. But I'm sure there is another place where Default:
is used as I suggested (two possible values, not the wording). But I can't find the page anymore... I don't think it's been edited, maybe moved. Or I'm not looking in the right place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had thought we had another place too, but I didn't immediately find it and didn't want to introduce inconsistency! If you can't find it either, maybe it didn't make it into the new docs!
I'm happy to go with your suggestion if you feel it's more helpful! My only slight nagging concern is that output: 'static'
is a default that no one would set. And, it's def true by default default, even if they don't know what output: 'static'
is. Like, this is really the default. And I do think there's value in it being presented that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the example I was thinking about, but I finally found a page where Default:
uses two values:
Default: "development" when running astro dev, "production" when running astro build
( https://deploy-preview-10062--astro-docs-2.netlify.app/en/reference/programmatic-reference/#mode )
So I think something like this might be useful here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I only just saw your comment now... (thanks the email notifications...) I was on the page, but it seems I'm having auto-refresh issues with Github the last few days (notifs in particular). Even after posting the link, your comment wasn't displayed...
I'm happy to go with your suggestion if you feel it's more helpful! My only slight nagging concern is that
output: 'static'
is a default that no one would set. And, it's def true by default default, even if they don't know whatoutput: 'static'
is. Like, this is really the default. And I do think there's value in it being presented that way.
Yeah, I understand your concern and honestly I don't have a perfect solution. Considering the way the content is presented, I would say it is fine the way it is. But then I think that some people read the doc too quickly and they risk stopping at the word in bold (Default:
) without reading the rest and that could cause confusion.
So I thought I'd just mention it just in case. Now, especially since we're not sure what's best, I also agree to leave it as is, and if it's a problem, we'll update later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the conversation! I'm leaning towards leaving it like this and then waiting to see whether it causes confusion or more people mention it. It can always be changed!
I think part of why I have this opinion is the reason behind making the old hybrid behaviour the default is because too many people were simply enabling output: 'server'
just to have ANY on-demand rendering features, even when they only needed it on one page. We're also trying to position the docs to nudge towards the behaviour of thinking of your site as prerendered, opting in per-route when you need it, and only when and if you realize you really need it, switching to server
mode. I do think this framing helps that story as well!
If you look at how the labels are implemented, it's not that simple! 😅 That's what I meant when I said it's not easy to get it sync'd all down the page. The tabs don't all share the same label because the tabs themselves are the
|
Oh! So I was right to think that I might not have understood the end of your sentence... I hesitated to clarify it. 😅 Thanks for the explanation, now I understand! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good @sarah11918! I bow down to your indomitable spirit in taming the reference wilderness.
I do agree with @ArmandPhilippot re: the “default” field for prerender
potentially being confusing given it varies depending on mode, but will leave it to your discretion whether you want to address that or are happy leaving it as is.
Description (required)
Combines the
Astro.global
andcontext
object entries since several are duplicates or only links up to the global entry anyway.This also makes each entry more visible to see where we haven't sufficiently documented.
Also:
getStaticPaths()
contentprerender
partial
Astro.self
andAstro.slots
to the Template Reference page, as a second item to go with JSX-like expressions, since that's where this content is most similarLinks pass checks, but follow up for error reference will be needed once we are sure we are done.
We could further:
api-reference
-- will give us more links to fix, but will be more accurate? Not needed, and we can always tackle this with our few pages that are still left in/basics/
.