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

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

Merged
merged 3 commits into from Aug 22, 2019

Conversation

@SukkaW
Copy link
Member

SukkaW commented Aug 15, 2019

What does it do?

Remove twitter:title and twitter:description which is duplicated with og:title and og:description

According to the Twitter Card Docs:

When using Open Graph protocol to describe data on a page, it is easy to generate a Twitter card without duplicating tags and data. When the Twitter card processor looks for tags on a page, it first checks for the Twitter-specific property, and if not present, falls back to the supported Open Graph property.

SInce the value of twitter:title and twitter:description is the same as og:title and og:description, they can be removed.

How to test

git clone -b BRANCH https://github.com/USER/hexo.git
cd hexo
npm install
npm test

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 15, 2019

Coverage Status

Coverage decreased (-0.002%) to 97.152% when pulling 65757c9 on SukkaW:remove-duplicate-og-tag into 3176fe0 on hexojs:master.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Aug 15, 2019

please remove relevant lines in "test/scripts/helpers/open_graph.js".

test/scripts/helpers/open_graph.js Outdated Show resolved Hide resolved
@SukkaW SukkaW force-pushed the SukkaW:remove-duplicate-og-tag branch from 86316dd to b029921 Aug 15, 2019
@curbengh curbengh changed the title fix: remove duplicated twitter card tags fix(open_graph): remove duplicate twitter card tags Aug 17, 2019
result += meta('twitter:title', title);
if (description) {
result += meta('twitter:description', description, false);
}

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

This comment has been minimized.

Copy link
@YoshinoriN

YoshinoriN Aug 20, 2019

Member

twitter:image is deletable. Isn't it?

This comment has been minimized.

Copy link
@SukkaW

SukkaW Aug 20, 2019

Author Member

@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.

This comment has been minimized.

Copy link
@curbengh

curbengh Aug 21, 2019

Contributor

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.

This comment has been minimized.

Copy link
@YoshinoriN

YoshinoriN Aug 21, 2019

Member

@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.

This comment has been minimized.

Copy link
@curbengh

curbengh Aug 22, 2019

Contributor

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.

@YoshinoriN YoshinoriN merged commit b402da0 into hexojs:master Aug 22, 2019
3 of 4 checks passed
3 of 4 checks passed
coverage/coveralls Coverage decreased (-0.002%) to 97.152%
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@YoshinoriN YoshinoriN added this to the v4.0.0 milestone Aug 22, 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.