Skip to content

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Jun 5, 2024

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)

Q A
Bug fix? no
New feature? yes
Issues Fix #...
License MIT

@carsonbot carsonbot added Feature New Feature Icons Status: Needs Review Needs to be reviewed labels Jun 5, 2024
@smnandre smnandre requested a review from kbond June 5, 2024 05:06
Copy link
Contributor

@WebMamba WebMamba left a 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

@smnandre
Copy link
Member Author

smnandre commented Jun 5, 2024

Why do you think we need an interface?

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.

We don't have any plant for other IconRenderer?

No, same for the Router, Serializer, FormBuilder, etc.. it makes testing and autowiring easier for users :)

And why user would need to create their own Renderer?

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)

I think this a bit to early for this abstraction

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 ^^)

@smnandre smnandre requested a review from WebMamba June 6, 2024 05:35
@kbond
Copy link
Member

kbond commented Jun 14, 2024

In my packages, I'd make IconRenderer autowireable but in Symfony core, I believe interfaces are preferred, even if there's only 1 implementation. It's easier for someone to decorate.

@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?

@smnandre
Copy link
Member Author

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

Copy link
Member

@kbond kbond left a 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.

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jun 14, 2024
@kbond
Copy link
Member

kbond commented Jun 26, 2024

@WebMamba, have we adequately addressed your concerns?

@smnandre
Copy link
Member Author

smnandre commented Jul 5, 2024

@WebMamba ?

@javiereguiluz javiereguiluz force-pushed the icon/renderer-interface branch from 5df63ca to 22cfef0 Compare July 11, 2024 13:17
@javiereguiluz
Copy link
Member

Let's merge this one because the given arguments were compelling.

Thanks @smnandre!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Icons Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants