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

Should allow publisher to be a Person #304

Closed
ScottKillen opened this issue Aug 7, 2018 · 17 comments
Closed

Should allow publisher to be a Person #304

ScottKillen opened this issue Aug 7, 2018 · 17 comments

Comments

@ScottKillen
Copy link

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

@aav7fl
Copy link
Contributor

aav7fl commented Aug 29, 2018

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
Copy link

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

@jekyllbot
Copy link
Contributor

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
Copy link

Has this been fixed?

@jekyllbot jekyllbot removed the stale label Oct 29, 2018
@ScottKillen
Copy link
Author

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
Copy link

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
Copy link
Contributor

aav7fl commented Oct 29, 2018

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
Copy link
Author

Structured Data Testing Tool allows publisher to be a person.

2018-10-29 15_36_11-window

@aav7fl
Copy link
Contributor

aav7fl commented Oct 29, 2018

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
Copy link

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
Copy link
Author

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

  • Why the priority on AMP? Jekyll doesn't support it out of the box and it can be a lot of work to implement.

@nickserv
Copy link

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
Copy link
Contributor

aav7fl commented Oct 30, 2018

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.

#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
Copy link
Author

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
Copy link
Contributor

aav7fl commented Oct 30, 2018

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
Copy link
Author

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
Copy link
Contributor

aav7fl commented Oct 30, 2018

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

@jekyll jekyll locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants