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 icon to rss2.xml #102

Merged
merged 10 commits into from Nov 5, 2019

Conversation

@sehajyang
Copy link
Contributor

sehajyang commented Oct 30, 2019

I configured feed.icon in the config.yml file.
but that icon was not exposed when creating the feed xml file.
So, I add in rss2.xml :

<url> {{(url + config.feed.icon) | uriencode}} </ url>

and I see that it works.

I configured `feed.icon` in the `config.yml` file but that icon was not exposed when creating the feed xml file.
So, I Add in rss2.xml `<url> {{(url + config.feed.icon) | uriencode}} </ url>` and I see that it works.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 30, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 417898d on sehajyang:patch-1 into d169992 on hexojs:master.

Copy link
Member

SukkaW left a comment

After this change, it will become impposible to use third-party url of feed icon.

Please use full_url_for() instead.

@sehajyang

This comment has been minimized.

Copy link
Contributor Author

sehajyang commented Oct 30, 2019

full_url_for() seems to add the blog url as a prefix.
So if it's a third-party url, it seems to be blog url + third-party url. am I right?

@sehajyang

This comment has been minimized.

Copy link
Contributor Author

sehajyang commented Oct 30, 2019

@sukka

I installed hexo-util and added the following code to generator.js:

const all_url_for = require ('hexo-util'). all_url_for.bind (hexo);

var icon;
if (config.email) {
    icon = gravatar (config.email)
} else {
    icon = all_url_for (config.feed.icon)
}

However, there was an error where config could not be found in all_url_for ().
Also bind (hexo); didn't work. I tried a few ways to solve this problem but it failed.
How can i use all_url_for?

@SukkaW

This comment has been minimized.

Copy link
Member

SukkaW commented Oct 30, 2019

@sehajyang Try this?

First, require full_url_for at the beginning:

const { encodeURL, gravatar } = require('hexo-util');

 const { encodeURL, gravatar, full_url_for } = require('hexo-util'); 

Then change L38:

if (feedConfig.icon) icon = url + encodeURI(feedConfig.icon);

to:

if (feedConfig.icon) icon = encodeURI(full_utl_for.call(this, feedConfig.icon)); 

You can also include shorter object syntax in this PR:

const xml = template.render({
config: config,
url: url,
icon: icon,
posts: posts,
feed_url: config.root + feedConfig.path
});

to

 const xml = template.render({ 
   config, 
   url, 
   icon, 
   posts, 
   feed_url: config.root + feedConfig.path 
 }); 
@sehajyang sehajyang requested a review from SukkaW Oct 30, 2019
@sehajyang

This comment has been minimized.

Copy link
Contributor Author

sehajyang commented Oct 30, 2019

@SukkaW
I fixed it as your comment and saw that it works.
Thanks to your comment I was able to solve this issue. Thank you for your help 😊

@SukkaW
SukkaW approved these changes Oct 30, 2019
lib/generator.js Outdated Show resolved Hide resolved
rss2.xml Outdated Show resolved Hide resolved
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 3, 2019

Unit test is also missing, let me know if you need help creating one.

@sehajyang

This comment has been minimized.

Copy link
Contributor Author

sehajyang commented Nov 4, 2019

@curbengh
Yes I need help. Can you make one?
Also, I modified the part you commented above. Let me know if there's anything I need to change.

@sehajyang sehajyang requested a review from curbengh Nov 4, 2019
rss2.xml Show resolved Hide resolved
curbengh and others added 4 commits Nov 5, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 5, 2019

I just added two tests, for atom only. Simply duplicate them (not replace), change the necessary config to rss2 then change the corresponding expected result.

A brief guide for using cheerio; atom.xml starts with the following value,

<feed xmlns="https://www.w3.org/2005/Atom">
  <title>{{ config.title }}</title>
  <icon>{{ icon }}</icon>

Notice $('feed>icon') corresponds to the <feed> and <icon> elements. The > is a shortcut for .children(); in this case, we're looking for <icon> in the children of <feed>.

Hint: for subsequent levels,

<feed xmlns="https://www.w3.org/2005/Atom">
  <title>{{ config.title }}</title>
  <foo>
    <bar>baz</bar>
  <foo>
$('feed>foo>bar').text()
// baz
atom.xml Outdated Show resolved Hide resolved
This reverts commit 07f8b70.
@sehajyang

This comment has been minimized.

Copy link
Contributor Author

sehajyang commented Nov 5, 2019

@curbengh
I created the rss2 icon's test code with your comments and the atom icon's test code.
Thank you for your help.
Please let me know if there are any errors.

@sehajyang sehajyang requested a review from curbengh Nov 5, 2019
@curbengh curbengh merged commit d168026 into hexojs:master Nov 5, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@sehajyang sehajyang deleted the sehajyang:patch-1 branch Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.