Skip to content

Conversation

mqus
Copy link
Contributor

@mqus mqus commented Nov 6, 2019

This copies the default rss template from zola and changes the <description> tag of all pages from page.summary to page.content to show the whole article in rss readers and not only the summary (which, in case of the newsletter, only explains the concept of the newsletter and shows links to the discussion, but does not summarize this specific newsletter itself, which I find pretty useless). For comparison, This Week in Rust also puts the fulltext articles in the rss-feed.

I have tried the change locally and it does generate the intended rss.xml,
but I didn't check if it also renders as nice as intended (But I'm pretty sure it will, I'll test this soon.)

I'm also not sure how smooth the transition between these two rss feeds has to be.

This copies the default rss template from zola and changes the <description> tag of all pages from page.summary to page.content to show the whole article in rss readers and not only the summary (which, in case of the newsletter, only explains the concept of the newsletter and shows links to the discussion, but does not summarize this specific newsletter itself)
@mqus mqus requested a review from a team as a code owner November 6, 2019 16:08
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @andre-richter (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@mqus
Copy link
Contributor Author

mqus commented Nov 6, 2019

I checked the rendering in Feeder and It looks fine. The tables are not rendered at all, it just displays the table as one line per cell, as if it would just ignore all table-related html tags. It's readable and understandable, but not really pretty, but in my opinion, this is an acceptable tradeoff. Of course, other rss-readers can render this differently.

@andre-richter
Copy link
Member

@rust-embedded/resources anybody feels like taking the review token here? Description reads good, but then I have zero history maintaining the blog infra...

@therealprof
Copy link
Contributor

@andre-richter This is really just configuration. It looks good to me but I really can't properly test it. We could simply apply it, try it out with the real feed once deployed and back it out in case it's not improvement.

Copy link
Member

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

SGTM, let's merge and see if we get any complaints :)

bors r+

bors bot added a commit that referenced this pull request Nov 8, 2019
75: Make the RSS-feed fulltext r=jamesmunns a=mqus

This copies the [default rss template from zola](https://github.com/getzola/zola/blob/master/components/templates/src/builtins/rss.xml) and changes the `<description>` tag of all pages from `page.summary` to `page.content` to show the whole article in rss readers and not only the summary (which, in case of the newsletter, only explains the concept of the newsletter and shows links to the discussion, but does not summarize this specific newsletter itself, which I find pretty useless). For comparison, This Week in Rust also puts the fulltext articles in the rss-feed.

I have tried the change locally and it does generate the intended `rss.xml`,
but I didn't check if it also renders as nice as intended (But I'm pretty sure it will, I'll test this soon.)

I'm also not sure how smooth the transition between these two rss feeds has to be.

Co-authored-by: Markus Richter <8398165+mqus@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Nov 8, 2019

Build succeeded

@bors bors bot merged commit 820793f into rust-embedded:master Nov 8, 2019
@mqus
Copy link
Contributor Author

mqus commented Nov 8, 2019

Thanks!

@mqus
Copy link
Contributor Author

mqus commented Nov 30, 2019

It doesn't seem to propagate to the site...
I generated rss.xml locally and it worked fine. If I remember it right then I just used the command from the CI script.
So why hasn't the rss.xml file changed for the last release?

@jamesmunns
Copy link
Member

@mqus what version of Zola are you using locally? Maybe we need to update the version in CI?

@mqus
Copy link
Contributor Author

mqus commented Nov 30, 2019

0.9.0, but according to the changelog of zola (https://github.com/getzola/zola/blob/master/CHANGELOG.md) there weren't any relevant changes in recent releases... I'll test it again as soon as I can and try to look into it.

@mqus
Copy link
Contributor Author

mqus commented Nov 30, 2019

I downgraded zola to 0.5.1 (which should be the version the ci uses) and it creates the rss.xml just like the ci and differently than 0.9.0. so it seems I was wrong and It is a version issue... I'll see if I can get it to work in 0.5.1

@jamesmunns
Copy link
Member

You can also send a PR to update CI to 0.9.0, no objections here, thanks again for following up with this!

@mqus mqus mentioned this pull request Dec 1, 2019
@mqus
Copy link
Contributor Author

mqus commented Dec 1, 2019

I'd rather not update CI, as it does seem to break some subtle things (such as rss) and I'm not that involved with zola or the rust embedded blog, but if you do the step in the future I'll match the rss file.

bors bot added a commit that referenced this pull request Dec 1, 2019
86: Fix rss template r=jamesmunns a=mqus

This commit fixes the rss fulltext template introduced in #75 which was originally build under the assumption of zola v0.9.0.
For v0.5.1 the template has to be in a different location and can't use `escape_xml` as that was introduced in later versions.
This should do no harm because the links didn't contain xml in the first place.

Co-authored-by: Markus Richter <8398165+mqus@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants