Skip to content
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

Add feed url attributes to content objects #2513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add feed url attributes to content objects #2513

wants to merge 1 commit into from

Conversation

bryanbrattlof
Copy link
Contributor

@bryanbrattlof bryanbrattlof commented Jan 11, 2019

Hi everyone!

After my first try (#2510) I found @avaris comment that I thought was a much better approach to this, but had a few questions I wanted to run past everyone.

The main idea from that thread was that we should try to avoid theme creators generating their own urls manually. Mainly to prevent things from breaking when changes are made in the future. Like Atom and RSS Feed settings :)

This fix will add atom_feed_url and rss_feed_url attributes to the Tag, Category, and Author objects, as well as add lang_rss_feed_url and lang_atom_feed_url to the Content object.

I've also included updates to the 'simple' theme to use these attributes, that my first attempt fixed, but didn't address the real issue of building urls in the themes directly.

This does not update the documentation, or setup any tests to make sure this works properly mainly because I wasn't sure what to do about the FEED_ATOM, FEED_RSS, FEED_ALL_ATOM, FEED_ALL_RSS settings. To me it would be wierd to have:

<link href="{{ FEED_DOMAIN }}/{{ FEED_ALL_ATOM }}" />

for all content, and

<link href="{{ FEED_DOMAIN }}/{{ tag.atom_feed_url }}" />
<link href="{{ FEED_DOMAIN }}/{{ article.lang_atom_feed_url }}" />
...

for each category and translation.

My plan was to get an idea of what everyone though before I finish the documentation and tests.

So here are my questions for some of the more veteran developers.

  • Why don't we treat translations like we do with Categories and Tags by having a Translation object? Would that make sense if we did?

  • What should we do about the FEED_ATOM, FEED_URL settings? Should they stay as they are and let theme developers use them directly? or should we have a Feeds object passed as another Common Variable to all the templates? (eg: feed.atom_all_url)

Going the common variable route would give us the ability to prevent using the settings that don't need formatting in the theme directly. So that's a plus

Sorry for the wall of text.
Hope everyone is having a great 2019!

@oulenz
Copy link
Contributor

oulenz commented Feb 24, 2019

I don't have any experience with feeds, so I can't say much about this.

I'm trying to understand your suggestion to introduce a Translation object. I don't see how they are comparable to Categories and Tags. Translations are individual articles or pages. The purpose of the Category, Tag and Author objects is essentially to generate pages that list all the articles with that category/tag/author.

Or do you mean something like a Language object, to generate pages with all the content in that language?

@bryanbrattlof
Copy link
Contributor Author

Thanks @oulenz,

I've made some simple tests and I'm working on the documentation now for this pull request. I'll be ready to push updates to this by the end of this weekend.

My thinking was more along the lines of the Language object you're talking about.

I was thinking that a Translation object could be used to collect all the articles and pages that use the same language, much like the Tag object is used to collect all the articles with the same tag.

The difference though would be that the Translation object would come from different source files, so this might be harder than I originally thought to link the different translations of the same article together.

The more I play on this branch though, the more I'm thinking that I should stick to introducing these properties before I work on anything like that, as its not really related to this pull request

Thanks again

@justinmayer
Copy link
Member

@avaris: Could you take a look at this pull request and provide your assessment of its potential value to the project? If you think it's worthwhile, what would need to be done in order to merge it?

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.

None yet

4 participants