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

Update to use JMicrodata library rather than hard coded values #8933

Closed
wants to merge 7 commits into from

Conversation

photodude
Copy link
Contributor

We have the JMicrodata library, lets actually use it.

Testing

  1. Add patch
    https://docs.joomla.org/Testing_Joomla!_patches
  2. modify the template to remove any overrides for com_content or mod_articles_latest (you can't test if you have an override, since you would just be testing the override)
  3. Add a latest articles module to site display
  4. add some articles, arcived articles and blog layout menu types for displaying on the site side
  5. Check that microdata values are still applied using a microdata validation tool
    https://developers.google.com/structured-data/testing-tool/

We have the JMicrodata library, lets actually use it.
@photodude photodude changed the title Update to use JMicrodata library rather than hard coded values Update latest articles module to use JMicrodata library rather than hard coded values Jan 18, 2016
@photodude photodude changed the title Update latest articles module to use JMicrodata library rather than hard coded values Update to use JMicrodata library rather than hard coded values Jan 19, 2016
@wilsonge
Copy link
Contributor

I mean this was a deliberate decision to hardcore (see discussion at #3358). Whilst I'm open to reviewing those kinds of decisions I don't think much (if anything) has changed since then. But willing to hear arguments otherwise :)

@photodude
Copy link
Contributor Author

Thanks @wilsonge I tried to find the justification, or PR, for hardcoding the microdata, but I didn't come accross #3358
I'll look over that discussion before continuing.

@ghost
Copy link

ghost commented Jan 20, 2016

Using library would be a nice first step to get rid of microdatas in HTML at all if there would be a simple single switch somewhere to suppress output.

For guys that use JSON-LD (more flexible than microdata if Google changes structured datas rules (again and again and again)). Now you have to create several overrides or have to remove microdata by plugin. Not possible to show them only robots. And so on...

Just saying that

@ggppdk
Copy link
Contributor

ggppdk commented Jan 21, 2016

The JMicrodata api could be made more readable
e.g. display(...) is commonly called without parameters

i suggest that these are added:

public function __toString()
{
    return $this->display();
}

public function itemprop($property)
    return $this->property($property);
}

so that

 '<span' . echo $microdata->property('name')->display(); . '>';

it can be made (which is more readable and searchable):

 '<span' . echo $microdata->itemprop('name'); . '>';

@Bakual
Copy link
Contributor

Bakual commented Jan 21, 2016

There was a PR to include a plugin which had a quite simple markup to then generate proper microdata: #4118

@brianteeman
Copy link
Contributor

Setting to needs review so the cms maintainers can make a decision based on comment above by @wilsonge


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

@sandstorm871
Copy link

Tested the patch anyway (just noticed its set to needs review) & after patch, each article title now has a ">" at the start of it so, I guess the patch needs cleaning up before testing further.


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

@photodude
Copy link
Contributor Author

@sandstorm871 Can you give additional details on where your seeing titles with additional ">" at the start?
I have reviewed a site with the Joomla sample data and this patch and I'm unable to comfirm the reported issue with this patch. (tested the Joomla defult templates and a custom template with HTML overrides removed)

Maybe there is a template or plugin specific issue causing the addional ">" at the start of the title...

@photodude
Copy link
Contributor Author

@ggppdk I agree The JMicrodata api could should be made more readable

@sandstorm871
Copy link

http://www.mynewsite.me.uk/ace
With patch there is an additional > at the start of the title.
If I revert patch the > has gone

@photodude
Copy link
Contributor Author

@sandstorm871 Thanks. I'm not sure what's happening but, I now can see it's with the mod_articles_latest changes.

I'll see if I can pin down what is at the root of that extra caret (or pull those changes from the PR).

Nuances of using the JMicrodata plugin, the documentation needs to be clearer on proper useage of the different parameters.
@photodude
Copy link
Contributor Author

@sandstorm871 mod_articles_latest changes was the first item I changed. Looks like I had used a content('')-> in the setup for the JMicrodata which resulted in an additional span being added in the code resulting in a tag ending mismatch.

There are still plenty of nuances to the JMicrodata API I don't yet understand.

In anycase the reported extra caret > before mod_articles_lates article titles should be resolved
remove the current patch, update the patch tester list, and reapply this updated patch and it should work as expected.

@wilsonge
Copy link
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-6 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.

7 participants