search

sampart / BreadcrumbsBundle

A small breadcrumbs bundle for Symfony
Other
188 stars 68 forks source link

added JSON-LD template #75

Closed toooni closed 7 years ago

toooni commented 7 years ago

I didn't know if I should go any further to make this somehow configurable since this would create some BC breaks and this Bundle doesn't seem to be "crowded" at the moment.

richsage commented 7 years ago

Thanks!

I'm not sure I see the significance behind renaming from breadcrumbs.html.twig to microdata.html.twig, given that the former now just includes the latter with no extra changes, so I'd be happy to include the PR without that BC break in v1.

The documentation (README.md) should also be updated to mention the existence of the new json-ld.html.twig file so that users are made aware and then can include it in their layout templates if required.

toooni commented 7 years ago

Sorry. I didn't had much time with my last reply. I'll try to explain a bit further. The name breadcrumbs.html.twig would be irritating because there are now multiple formats to use. So Microdata is the correct definition according to schema.org. I do understand that this change would be a BC break in v1. This is the reason I implemented a breadcrumbs.html.twig which is falling back to microdata.html.twig and added a deprecation notice if somebody has manually configured or extended the breadcrumbs.html.twig. This way the deprecation message can already inform the user that microdata.html.twig should be used for v2. And as soon as v2 is released, the deprecation message and the breadcrumbs.html.twig can just be removed.

The question is also. Do you agree with the naming for v2? Or should I change it to something else? Sure I could keep breadcrumbs.html.twig but this wouldn't make sense since there is a json-ld.html.twig.

I'll be happy to hear if I should change the PR anyway. As soon as you give me the go for a solution, I'll change the README.md and notify you when the PR is ready again.

toooni commented 7 years ago

@richsage any thoughts on my comment?

sampart commented 7 years ago

Hi @toooni, sorry for the long delay on this one. I've picked this up now.

I've reviewed your changes and am happy to merge, if you can do three things:

Thanks Sam

sampart commented 7 years ago

@toooni Sorry, I didn't spot that you'd already changed the viewTemplate param in your first commit. You don't need that last commit, therefore - it's right for it to be microdata.html.twig. Could you delete commit 0969147?

Sorry for the confusion!

toooni commented 7 years ago

@sampart Thank you for getting back! I've done the changes as requested. I am not sure about the README.md. Please just tell me if you had something else in mind.

toooni commented 7 years ago

@sampart done.. Thanks!

sampart commented 7 years ago

Merged now. Thanks again, @toooni!