Skip to content

Add JMicrodata to popular articles module for schema.org microdata#8934

Closed
photodude wants to merge 1 commit intojoomla:stagingfrom
photodude:patch-9
Closed

Add JMicrodata to popular articles module for schema.org microdata#8934
photodude wants to merge 1 commit intojoomla:stagingfrom
photodude:patch-9

Conversation

@photodude
Copy link
Copy Markdown
Contributor

We have the JMicrodata library, lets actually use it.

Testing

  1. Add patch
    https://docs.joomla.org/Testing_Joomla!_patches
  2. Add a popular articles module to display
  3. Check that microdata values are applied using a microdata validation tool
    https://developers.google.com/structured-data/testing-tool/

@photodude photodude changed the title Add JMicrodata library for schema.org microdata Add JMicrodata for schema.org microdata Jan 18, 2016
@photodude photodude changed the title Add JMicrodata for schema.org microdata Add JMicrodata to popular articles module for schema.org microdata Jan 18, 2016
@sandstorm871
Copy link
Copy Markdown

I have tested this item ✅ successfully on e5481b1

Added "Most Read" Module after patch & confirm that Microdata is now shown in the validation tool.
J3.5 Beta2


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8934.

@anibalsanchez
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on e5481b1

Test OK


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8934.

@joomla-cms-bot
Copy link
Copy Markdown

This PR has received new commits.

CC: @anibalsanchez, @sandstorm871


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8934.

@photodude
Copy link
Copy Markdown
Contributor Author

@anibalsanchez, @sandstorm871 No actual change, just Rebased the branch to staging...

@anibalsanchez
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 96f41d7

Test OK


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8934.

@brianteeman
Copy link
Copy Markdown
Contributor

Set to needs review for same reason as #8933


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8934.

@ghost
Copy link
Copy Markdown

ghost commented Mar 9, 2016

This PR makes no sense.
Property name is not supported by Google.
Article without an Image less than 695px width is invalid.
Article without a publisher is invalid.
and so on.

Nearly all structured data markups in Joomla are (Google) invalid meanwhile because the required combinations of properties are nearly never given.

Thus, only unnecessary HTML/markup.

https://developers.google.com/structured-data/rich-snippets/articles#article_markup_properties

#8933 (comment)

@photodude
Copy link
Copy Markdown
Contributor Author

@bertmert This PR Just follows the same format already used in the hard-coded Joomla microdata for the latest articles module. The difference being the use of JMicrodata library rather than hard-coded structured data.

This PR is part of the Larger discussion in consideration for whether to use Hard-coded microdata for the structured markup or to use the JMicrodata library.

IMHO, Discussions about IF the existing "structured data markups in Joomla" are necessary or correct, are completely separate from the discussion that this and the other PR bring up. As such, your concerns would be more valuable discussed in the mailing group

@ghost
Copy link
Copy Markdown

ghost commented Mar 10, 2016

I understand but still think it's useless and a bad idea to change now already unsufficient/wrong Microdatas to JMicrodata.

If you want to remove/"deactivate" Microdata (in overrides) you have to find and remoive after PRs like this every single JMicrodata-Snippet to avoid PHP errors if you want to avoid loading of unneeded class JMicrodata in every file.

With hardcoded ones you just have to remove itemscope attributes to avoid conflicts with e.g. application/ld+json.

(BTW: It's more performant to leave them hardcoded. But just said.)

@photodude
Copy link
Copy Markdown
Contributor Author

@bertmert I'm not agreeing or disagreeing, I just feel there is a better place for that discussion which would have more overall impact.

As for performance, I would agree that hard coded microdata would have better performance on a "non-cached" site. In the same way that a fully hard coded HTML file will be faster than a CMS that is dynamically serving content and queries a database. But the logic to that argument against using a dynamic library, for an encoding that can be dynamic, just because there is a performance difference in a non-cached setup; falls a bit flat for me as I believe a CMS should be capable of being dynamic, overridable, and simple to the end user.

I haven't run the numbers, but I doubt the performance impact of using the JMicrodata library in a non-cached site would be noticeable or significant vs Hard-coded markup. But I'm willing to accept that I could be wrong. Testing that and proving that there is a noticeable or significant difference for using the JMicrodata library vs hard-coded markup (in non-cached and cached) could make a difference in the overall discussion (or it would prove that it's a non-issue). Unfortunately, I don't have the time to do the testing right now.

@ghost
Copy link
Copy Markdown

ghost commented Mar 10, 2016

Performance was just a little addition at the end of my post.

@photodude
Copy link
Copy Markdown
Contributor Author

@bertmert I took a look at your comment and the details in the supplied link

Property name is not supported by Google.
https://developers.google.com/structured-data/rich-snippets/articles#article_markup_properties

From the link I can see how you came to the conclusion that name is not a supported property for Google. It's easy to assume that name is not supported when it's not in the example. But that's misreading the example. You must look at the full schema.org/Article type to see what's supported. name is a parent property for articles from the schema.org/Thing type. This is why the example links to the full schema.org/Article type so you can read the full specification and not just the supplied example. You can see that name is a fully supported property for an article type by just running a test against Google's microdata validation tool

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented May 7, 2016

Myself, Chris, Roland and Viktor have just discussed this and I think our decision is to stick with hardcoding this for the same reasons we chose not to use it in the first place. I'm going to close this PR and the other module ones for this reason

@wilsonge wilsonge closed this May 7, 2016
@photodude photodude deleted the patch-9 branch May 7, 2016 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants