Skip to content

[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

Merged
merged 1 commit into from
Jul 11, 2024

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!

@javiereguiluz javiereguiluz merged commit 21a6afc into symfony:2.x Jul 11, 2024
5 checks passed
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