-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Improved rss readme #12157
Improved rss readme #12157
Conversation
🦋 Changeset detectedLatest commit: 46c1bf9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: Armand Philippot <[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.
Hey @bholmesdev this is fantastic! (And, makes me really want to see this as a proper integration 😅 )
Left some stray thoughts below! 🙌
packages/astro-rss/README.md
Outdated
## `rssSchema` | ||
Type: `Record<string, string> (optional)` | ||
|
||
An object mapping a set of `xmlns` suffixes to strings of metadata on the opening `<rss>` tag. |
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.
So two things here:
-
A second sentence like "This is used to/for" with plain language explanation of the purpose here would be nice.
-
If the URL is just an example, then standard practice is to use
example.com
EVERYWHERE, even when other URLs seem fun.
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 took me down a rabbithole to understand what xmlns
even is. Learned a lot! Updated the entry to give a practical 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.
I leave the final OK to Sarah :) once she gives the gree light, we can merge it
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Thanks @bholmesdev ! Left some follow up and I think you should have all the feedback you need from me to make your final judgement calls, and merge at will! 🫡 |
Changes
This tightens up the RSS README to remove unneeded guide pages (consistent with integration readmes) and reduce complexity of config options.
RSSFeedItem
docs underitems
content
object
Testing
N/A
Docs
Follow up from this discussion on the RSS docs.
Follow ups
We should remove the inline documentation for
item
options and link to this README as a single source of truth. This should prevent out-of-date options. We should also clearly link to this README to learn more about config options.