-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(next): routes resolved hook #10077
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.
Looks great @florian-lefebvre ! Just need to phrase this more like a "What should I do?" so I made a basic model, but you should update as necessary!
Oh, and are we documenting this on the Integrations API page too? Is that coming later? |
Co-authored-by: Sarah Rainsberger <[email protected]>
There you go 😄 |
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.
Some minor edits and suggestions below! We'll get this wrapped up well in time for a release tomorrow, no worries!
```ts | ||
interface IntegrationResolvedRoute { | ||
/** | ||
* The current **pattern** of the route. For example: |
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.
* The current **pattern** of the route. For example: | |
* The current **pattern** of the route, the path relative to the `src/pages/` directory and without a file extension. For example: |
This appears to be the pattern, right? Would you change this description?
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.
All those jsdoc come directly from core, and are also in astro:build:done
. So do you prefer not to update the jsdoc on that page at all, or update them in core and then reflect the changes 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.
Ah, OK. How important is it that these are kept identical to core? This is public facing docs, and one could argue should be written like docs.
(I was even thinking, this is a LOT of documenting going on inside a code sample. Why aren't we writing normal docs here?)
In that case, I'm OK to merge exactly what's already matching, but then think about at some point in the future this page becomes an actual docs page, not just copy paste of whatever you've written in code that I have no control over the quality of. 😅
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.
Yeah I think that's a good idea 👍
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
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.
See my comment about being OK to copy these as-is since this is apparently just a reproduction of what's in code.
BUT, these don't feel like the best docs to me. (Why is docs just a copy of code comments and not "documentation"?) We can leave that for some potential future exploration, whether this page needs itself an overhaul! And we don't have to block merging for the feature!
I think we can still take care of #10077 (comment) in this one tho! |
Sure! Can you propose an example that would go there? |
@sarah11918 i tried to add a little example, let me know what you think! |
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.
Just left the tiniest suggestion, but good to go whenever you're happy with it! (And, we can think about whether we want to improve this page overall in future!) 🚀
Co-authored-by: Sarah Rainsberger <[email protected]>
Description (required)
withastro/astro#12329
Related issues & labels (optional)