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.

Sign up for GitHub

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

fix(open_graph): remove duplicate twitter card tags #3668

Merged
merged 3 commits into from
Aug 22, 2019
Merged

fix(open_graph): remove duplicate twitter card tags #3668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fe5960
fix: remove duplicated twitter card tags
SukkaW Aug 15, 2019
b029921
test(open_graph): add og:locale & remove twitter:title
SukkaW Aug 15, 2019
65757c9
test(open_graph): remove twitter:description & update item
SukkaW Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions lib/plugins/helper/open_graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,6 @@ function openGraphHelper(options = {}) {
}

result += meta('twitter:card', twitterCard);
result += meta('twitter:title', title);
if (description) {
result += meta('twitter:description', description, false);
}

if (images.length) {
result += meta('twitter:image', images[0], false);
Copy link
Member

@yoshinorin yoshinorin Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

twitter:image is deletable. Isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yoshinorin It seems like og:image support multiple images while twitter:image only support one image. The docs from twitter do not mention multiple images usage.

Copy link
Contributor

@curbengh curbengh Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The doc is not exactly clear.

URL of image to use in the card. Images must be less than 5MB in size. JPG, PNG, WEBP and GIF formats are supported. Only the first frame of an animated GIF will be used. SVG is not supported.

"URL of image....Images must..."; image? images?

Anyway, the doc does mention twitter:image fallback to og:image, so I think it's redundant too.

Copy link
Member

@yoshinorin yoshinorin Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@SukkaW @curbengh

I found following comments from twitter developers forum.

According to comments. The twitter card use last og:image if multiple og:image tags exists. It's not following ogp specification.

They are very old comments (4~6 years ago). I could not find newer information. IMHO we should use twitter:image if we can not find more information about this matter.

I will merge this if other opinion does not exist after wait a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to comments. The twitter card use last og:image if multiple og:image tags exists. It's not following ogp specification.

I figure most users would expect the first image, which is the current behavior of open_graph.js. Let's keep twitter:image for now.

Expand Down
6 changes: 2 additions & 4 deletions test/scripts/helpers/open_graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ describe('open_graph', () => {
meta({property: 'og:url'}),
meta({property: 'og:site_name', content: hexo.config.title}),
meta({property: 'og:updated_time', content: post.updated.toISOString()}),
meta({name: 'twitter:card', content: 'summary'}),
meta({name: 'twitter:title', content: hexo.config.title})
meta({name: 'twitter:card', content: 'summary'})
].join('\n'));

return Post.removeById(post._id);
Expand Down Expand Up @@ -424,15 +423,14 @@ describe('open_graph', () => {
result.should.not.contain(meta({property: 'og:updated_time', content: '2016-05-23T21:20:21.372Z'}));
});

it('description - do not add /(?:og:|twitter:)?description/ meta tags if there is no description', () => {
it('description - do not add /(?:og:)?description/ meta tags if there is no description', () => {
const result = openGraph.call({
page: { },
config: {},
is_post: isPost
}, { });

result.should.not.contain(meta({property: 'og:description'}));
result.should.not.contain(meta({property: 'twitter:description'}));
result.should.not.contain(meta({property: 'description'}));
});

Expand Down