search

jekyll / jekyll-seo-tag

A Jekyll plugin to add metadata tags for search engines and social networks to better index and display your site's content.
https://jekyll.github.io/jekyll-seo-tag
MIT License
1.66k stars 294 forks source link

Should allow publisher to be a Person #304

Closed ScottKillen closed 6 years ago

ScottKillen commented 6 years ago

Publisher @type is hard-coded to be Organization, but Person should be allowed as well.

aav7fl commented 6 years ago

Original hard-coding maniac here. 👋

I'm not saying that was correct when I made that decision, but I was trying to make it compatible with the AMP spec. Previously we had no Publisher type so I wasn't looking at the alternatives too closely. If you can make a PR and demonstrate a non-breaking method of adding an option for person, I'm sure the maintainers would be happy to merge it in.

AMP Spec requires Publisher to be Organization. https://developers.google.com/search/docs/data-types/article#amp

nickserv commented 6 years ago

Could another field be added to make the choice of @type explicit, defaulting to Organization?

jekyllbot commented 6 years ago

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

nickserv commented 6 years ago

Has this been fixed?

ScottKillen commented 6 years ago

I understand @aav7fl reasoning above...and if the goal is to adhere to AMP spec then I guess this isn't an issue...and as the bot has indicated, there hasn't been any activity.

As for me, I don't care about the AMP spec and so I just discarded the plugin and implemented the functionality I wanted by hand.

nickserv commented 6 years ago

Out of curiosity, what's the issue with the AMP spec? I've been using this to improve SEO on Google using the Structured Data Testing Tool and Search Console, and I believe there was a warning telling me to use a different publisher field. I'd be surprised if Google and AMP had different recommendations.

aav7fl commented 6 years ago

Hey @nickmccurdy, I think I understand your question.

AMP has required schema markdown properties. Under the Required properties for AMP article pages (which includes blogs), it requires the publisher field. The only valid property is Organization.

ScottKillen commented 6 years ago

Structured Data Testing Tool allows publisher to be a person.

2018-10-29 15_36_11-window

aav7fl commented 6 years ago

Correct, but AMP structured data does not allow that property.

I understand this isn't an AMP repository. I don't see any reason you would get a PR rejected with the option to change the @type somewhere.

nickserv commented 6 years ago

Is there a way to have a separate schema for AMP, or should we just add an option so users can control whether they want more schema support or AMP compliance?

ScottKillen commented 6 years ago

I don't know how to ask this without sounding like I have an opinion...but I am really just curious:

nickserv commented 6 years ago

In general, I'm against supporting AMP because it's dominated by Google, not very useful even for Google SEo, and breaks the spec with things like this. We should prioritize spec support first, then maybe add an option for AMP if they want to use this plugin (or there can be a fork for AMP).

aav7fl commented 6 years ago

I made it AMP compatible because I wanted to, and no one had added many of the schema fields (nor had anyone volunteered to) before that point. That PR took a lot of rewrites before approval.

https://github.com/jekyll/jekyll-seo-tag/pull/151

It's quite useful for SEO. Nearly half of my results are to my AMP pages. It's had a significant boost on my site traffic. Same with page ranking. It usually pushes my posts a few spots higher.

We don't need to focus on an entire separate AMP layout either. Just make that single option and create a PR for it.

ScottKillen commented 6 years ago

Unfortunately, I am no Ruby programmer and I don't have the inclination to learn it while going through what you did with #151 on a project as prolific as this.

...and I mentioned that I have a solution that works just fine using liquid templates.

aav7fl commented 6 years ago

Nonsense, @ScottKillen. I bet you're a very capable developer. I didn't know Ruby before this project as well. Honestly, I still don't know it.

But back on point. I'm guessing a PR with a minor change for an option won't take too long.

ScottKillen commented 6 years ago

Thanks, @aav7fl

I think I could probably do it...but I've got to pick up Ruby.

On the positive side...I just read your article, Improving Jekyll…. Very valuable information...and I might even give AMP a try. Thanks!

aav7fl commented 6 years ago

No problem @ScottKillen! Let me know if you need any help. I'd be happy to answer any questions on it.