Add microdata markup #15
Conversation
| * @param boolean $isLast | ||
| * @return string | ||
| */ | ||
| protected function renderCrumb($name, $href, $isLast = false) | ||
| protected function renderCrumb($name, $href, $position, $isLast = false) |
There was a problem hiding this comment.
I'm not sure about this, since it breaks BC in the case when someone extends this class. Or is that usually a non-concern?
Can we move the argument to the end and use null by default? This way, it will only break code for people who actually override this particular method, and at least not for the people who just call it internally.
|
Hi @hmil ! First of all, thank you for the pull request, and forgive us for not replying for such a long time, there's really no excuse. I think it's a great idea, and we should definitely merge this, but could you first discuss the few issues I've mentioned in the comments? I'll give some thought about the BC break, but right now, I feel like just bumping the major version - it might be a BC break for at least someone, and I don't want to force BC breaks onto developers. Although I understand that a lot of people won't automatically have the update if they just do What do you think? |
|
Thanks for your comments. I didn't think about the "subclassing" use case. Anyway BC is broken because some CSS rules might fail due to the difference in HTML markup (although it doesn't break bootstrap for what it's worth). According to google's structured data testing tool, the "position" is not required. It is however used in the documentation so I'm not quite sure what's expected here. I changed the position argument as suggested and made it such that the tag for the position doesn't render if position is null (the other microdata attributes still do render anyway). Regarding the package version I'm not entirely sure. Bumping major is probably the safest option even though the update won't come "automatically" for most current users. (nb: It may be worth mentioning this feature in the readme) |
|
Hey, looks great now! Can you just remove the whitespace changes (trailing space removal) so we have a clean diff? I honestly have no idea how those trailing spaces got in there in the first place, but we're planning to clean up everything and enforce PSR-2 after this and #14 are merged, so we'd rather not have those changes in this PR at all. You can also squash everything if you'd like, though we don't insist on that, so whatever your personal preference is. Thank you! P.S. We don't have any dedicated "features" section in the readme, but if you find a good place to mention this, include that as well, it's useful for users to be aware of it. |
| @@ -395,10 +395,11 @@ public function removeAll() | |||
| * | |||
| * @param string $name | |||
| * @param string $href | |||
| * @param number $position | |||
There was a problem hiding this comment.
This should also be moved one line below.
|
Done. |
|
@hmil Thanks! |
Microdata markup for breadcrumbs allow search engines to locate a page within the website, just like humans do. There are other ways to do it but I believe it is best if this is handled at the core of the breadcrumbs plugin to avoid code duplication. This way all users get proper SEO out of the box 馃憤
This patch adds extra which may break too strict CSS rules so it might require at least a "minor" package version bump.
More information on breadcrumbs and structured data: https://developers.google.com/structured-data/breadcrumbs