-
-
Notifications
You must be signed in to change notification settings - Fork 15
[FEATURE] Make relative, absolute and links relative to a base URL possible #614
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this massive improvement!
In general this looks very good, however I would like to see an improvement that makes the UrlGenerator a bit more light weight. And that might reduce the number of changes you need to do.
Right now the UrlGenerator is basically handling 2 different strategies:
- Generate urls relative
- Generate urls absolute
If we would split this into 2 different classes, we could reduce the complexity of the UrlGenerator and also make it easier to understand.
Beside that you could replace the new UrlGenerator
in the RenderContext creation with an
injected UrlGeneratorInterface. This would make it easier to replace the UrlGenerator with a custom implementation.
This way the RenderContext does not require the settingsManager anymore. And we could
even inject the UrlGenerator everywhere we need it. To accomplish this
you could set the correct UrlGenerator in the GuidesExtension::load method, like you
do with the ThemeManager.
Another benefit of this approach is that we could inverse the dependency on the UrlGenerator
and RenderContext. This way the UrlGenerator can take what ever it needs from the RenderContext.
For example:
$urlGenerator->generateUrl($renderContext, $cannonicalUrl);
As an extra, you could allow people to configure the UrlGeneator in the Guides.xml.
<guides>
<urlGenerator class="My\Custom\UrlGenerator" />
</guides>
Or for buildin strategies:
<guides>
<urlGenerator strategy="relative" />
</guides>
19cfb9c
to
65cfda7
Compare
65cfda7
to
d366144
Compare
aa1081d
to
ff52f80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a single question. This looks good to go for me.
Thanks!
packages/guides/tests/unit/Renderer/UrlGenerator/RelativeUrlGeneratorTest.php
Show resolved
Hide resolved
ff52f80
to
2dc6366
Compare
And test it separately. Prepare to also handle relative URLs
This makes the menu easier to debug while testing as it is not all in one line
2dc6366
to
e39a1d5
Compare
depends on #607