-
-
Notifications
You must be signed in to change notification settings - Fork 356
[Icons] Expose IconRendererInterface #1889
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.
I am not sure about this? Why do you think we need an interface? We don't have any plant for other IconRenderer? And why user would need to create their own Renderer? I think this a bit to early for this abstraction
I prefer exposing an interface than a Twig Extension + TwigComponent to be fair ... it's more future-proof. And the contract is already ensured so that would not change a thing i guess.
No, same for the Router, Serializer, FormBuilder, etc.. it makes testing and autowiring easier for users :)
Not create their own... but decorate :) I personnaly would like to do it easily (as i prefer to wrap my icons in span for instance)
Again, as we offer a form of contract with the TwigExtension, i think we should make this official (i mean, only one method is not much ^^) |
In my packages, I'd make @smnandre, is an implementation that handles aliases in config (like requested in #1767) what we'd add? I'd like to implement #1800 at some point, does this interface prevent this? |
Nope it does not ! Alias/config handling is on the way (started on one of my branches) and the interface is perfect to abstract this implementation i guess Regardind SVG symbol / sprites / reuses / etc... It can be either coded via this interface or a new explicit method call but nothing blocking here |
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.
If we already have a need for an alternate implementation/decorator, I think the interface makes sense.
Making this autowireable allows rendering icons outside of twig - like provide the icon string in a json endpoint.
And why user would need to create their own Renderer?
I guess if the user wanted to take over the entire system this would be the place.
@WebMamba, have we adequately addressed your concerns? |
5df63ca
to
22cfef0
Compare
Let's merge this one because the given arguments were compelling. Thanks @smnandre! |
As the renderer won't change without BC (at least via the twig function/component).. i think it's ok to expose an interface here, allowing people to use it / decorate / etc..
Only thing i think we don't want (for now): add other public methods (like exposing the Icon object)