-
Notifications
You must be signed in to change notification settings - Fork 13
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
Simplicity and validation #10
Conversation
+1 |
Hi,
Thank you for your PR! I'm not home right now so I won't be able to review and test it until next week, but it's very appreciated :)
|
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.
H, and again sorry for the delay. I added a few comments to be addressed before the merge, but thanks a lot for the PR :) .
I noticed another small issue when validating the atom feed. It did not like the site URL not ending with a “/“. Therefore, I added a slash in the code. There is just one more warning at times but it is not a big deal. Sometimes, depending on the post (a post with no text, just an image, for example), the summary may end up being blank. The feed validator still marks the feed as valid, but gives a warning that there shouldn’t be blank summaries. In the future, some type of if/else coding could solve this... if summary is blank, add the text, “No summary found.” This way, there is never a blank summary. Here are screenshots of my feeds validating. As I state above in my replies, the couple changes you suggest are not necessary; the feeds are valid (you will see a warning about the utf-8 encoding but that’s just something with my server. Others may not get that warning... RSS valid: ATOM valid: |
Co-Authored-By: Amaury Carrade <amaury@carrade.eu>
Added html attribute and changed summary to full HTML content.
Changed description to full HTML content.
Please review my latest changes. Here is what I have done:
|
One should never change anything in the
Thanks for your contributions! |
These changes make things simple, ensure the feeds validate, and keep the feed post content text only versus trying to include HTML. This last part is admittedly a personal preference. I do not think we need the feeds to include the full HTML for posts. Plain text descriptions/summaries should suffice and help to ensure validation and feed reader compatibility.
The main reason I started making these changes is because the ID portion of the atom feed wasn’t validating. It was only pulling in the post ID which happens to be “1” every time. This pull request makes the ID the full permalink which validates.