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

Add microdata to breadcrumbs #7613

Closed
wants to merge 4 commits into from
Closed

Add microdata to breadcrumbs #7613

wants to merge 4 commits into from

Conversation

n9iels
Copy link
Contributor

@n9iels n9iels commented Aug 2, 2015

This PR add Micro Structured Data to the Joomla! breadcrumbs. I followed this guide lines: https://developers.google.com/structured-data/breadcrumbs

Also add some code style and change some if ($condition) { to if ($condition) : to make the code more readable.

Test instructions

Confirm that all parameters are working correct like they used do

@Fedik
Copy link
Member

Fedik commented Aug 2, 2015

looks good,
but changing ul to ol can have side effect for people who use ul as selector in the breadcrumbs styling, I think better keep ul

and I do not see <meta property="position" content="<POSITION_NUM>">, that need for each item 😉

@n9iels
Copy link
Contributor Author

n9iels commented Aug 2, 2015

@Fedik Thanks for noticing that :)

I add the <meta property="position" content="<POSITION_NUM>"> with help of the $key variable. But because it has to start with 1 instead of 0, I use $key + 1. I'm not sure if this is best way or not. Is there someone who think there is a better way to do this?

@Fedik
Copy link
Member

Fedik commented Aug 2, 2015

thanks!
$key + 1 is fine 😉

@ghazal
Copy link
Contributor

ghazal commented Aug 4, 2015

Test OK.
I am not very much into microdata, but the HTML generated looks allright.

@zero-24
Copy link
Contributor

zero-24 commented Aug 4, 2015

RTC Thanks 😄


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 4, 2015
@peterlose
Copy link
Contributor

Tested with Google's Structured data tester:

skaermbillede 2015-08-04 kl 13 02 55

@Fedik
Copy link
Member

Fedik commented Aug 4, 2015

hm, @peterlose on your test result still missed position

@n9iels
Copy link
Contributor Author

n9iels commented Aug 4, 2015

Strange, my test results where good. Did you fetch the last data?

@peterlose
Copy link
Contributor

Shoundn't position be itemprop as well? meta property is for RDFA

<meta itemprop="position" content="x" />

@Fedik
Copy link
Member

Fedik commented Aug 4, 2015

@peterlose right, I just noticed this also 😄
@n9iels sorry, I gave you the wrong example, before

Changed it, thanks for notice!
@n9iels
Copy link
Contributor Author

n9iels commented Aug 4, 2015

Changed it, thx for testing

@Fedik
Copy link
Member

Fedik commented Aug 4, 2015

@n9iels thanks, now is better 😉

@Kubik-Rubik Kubik-Rubik added this to the Joomla! 3.4.4 milestone Aug 6, 2015
@Kubik-Rubik
Copy link
Member

Thank you @n9iels and testers. Merged with ab3de64

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 6, 2015
@n9iels n9iels deleted the breadcrumb-microdata branch August 6, 2015 08:24
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.

None yet

7 participants