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

[shopsys] BreadcrumbGenerators are now automatically registered in BreadcrumbResolver #1141

Closed

Conversation

@TomasLudvik
Copy link
Member

TomasLudvik commented Jun 18, 2019

Q A
Description, reason for the PR Registering of new BreadcrumbGenerator was too painful. Users had to overwrite BreadcrumbResolverFactory, update services.yml etc.
New feature Yes
BC breaks Yes
Fixes issues closes #919 closes #1090 closes #1111
Have you read and signed our License Agreement for contributions? Yes
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch from 4e0395c to 586e2be Jun 18, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-url-provider branch from beae24c to 4e89723 Jun 18, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch from 586e2be to 558c3b1 Jun 18, 2019
@TomasLudvik TomasLudvik changed the title BreadcrumbGeneratos are now automatically registered in BreadcrumbResolver [shopsys] BreadcrumbGenerators are now automatically registered in BreadcrumbResolver Jun 18, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch from 558c3b1 to edde683 Jun 18, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-url-provider branch from 4e89723 to 9ffb0f4 Jun 18, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch 2 times, most recently from 2e73d5d to 0f8104c Jun 18, 2019
@PetrHeinz

This comment has been minimized.

Copy link
Contributor

PetrHeinz commented Jun 19, 2019

might be a duplicate of #1111

Copy link
Contributor

LukasHeinz left a comment

@TomasLudvik please check the related PR.

If it is the same, please choose which is better, and improve it from experience from the second one (if it makes sense). If its not the same, please explain differences.

After that I will move it to backlog.

@TomasLudvik

This comment has been minimized.

Copy link
Member Author

TomasLudvik commented Jun 19, 2019

Hello @LukasHeinz, the #1111 is WIP but this one is complete development prepared for code review. That is why I prefer this one.

…riendly_url_provider' when they implement FriendlyUrlDataProviderInterface
@TomasLudvik TomasLudvik force-pushed the tl-automatic-url-provider branch 2 times, most recently from 5af802d to 76736e3 Jun 27, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch 2 times, most recently from 15424ef to dd7ff4e Jun 27, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-url-provider branch from eef6a2d to c943c53 Jun 28, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch 3 times, most recently from 661632f to 05b7753 Jun 28, 2019
Copy link
Contributor

vitek-rostislav left a comment

Hi, nice job, just a few suggestions 😉

@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch from 34628ec to d4d4d5b Jun 28, 2019
Copy link
Contributor

vitek-rostislav left a comment

Hi, I have reviewed and tested it, works like a charm 👌

I have just two suggestions to clear things up in the instructions for breadcrumb generator and then you can move the task to validation 😉

@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch from f0fffd3 to fcf8862 Jun 28, 2019
…iner

- documentation about friendly url has been added
@TomasLudvik TomasLudvik force-pushed the tl-automatic-url-provider branch from 8514cbd to 6ac60d1 Jun 28, 2019
…solver

- documentation about breadcrumb has been added
@TomasLudvik TomasLudvik force-pushed the tl-automatic-breadcrumb-resolver branch from fcf8862 to 854c016 Jun 28, 2019
@TomasLudvik TomasLudvik force-pushed the tl-automatic-url-provider branch from 6ac60d1 to 524dffd Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.