Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Icons] Add support for int/float attribute to ux_icon function #2149

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

Kocal
Copy link
Collaborator

@Kocal Kocal commented Sep 11, 2024

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

I'm not really sure why do we have checks on attributes values, do you remember why @kbond? 🙏🏻

Copy link
Collaborator

@smnandre smnandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a bug fix..

But i'm fully open to discuss the feature if there is a real usage behind :)

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Sep 11, 2024
@smnandre smnandre changed the title [Icons] Add support for int/float attribute to Icon component [Icons] Add support for int/float attribute to ux_icon function Sep 12, 2024
@smnandre
Copy link
Collaborator

This works already

<twig:ux:icon height="48" width="64" />

So the problem is when you want to pass some dynamic value (and i really would love to have an example)

<twig:ux:icon height="48" width="{{ 64 / 2 }}" />

But i'm not ok to change it in Icon ... as a SVG tag does not have "float" or "int" values, but only strings.

So maybe we can (string) values in the Renderer :|

@Kocal
Copy link
Collaborator Author

Kocal commented Sep 12, 2024

This works already

<twig:ux:icon height="48" width="64" />

So the problem is when you want to pass some dynamic value (and i really would love to have an example)

<twig:ux:icon height="48" width="{{ 64 / 2 }}" />

Yes, that's what is described in issue #2148.

But i'm not ok to change it in Icon ... as a SVG tag does not have "float" or "int" values, but only strings.

In absolute terms, all the values you pass to HTML attributes are strings, but are then interpreted specially.
For example, <input type="range"> which accepts two attributes min and max, you pass strings but they need to be a valid (stringified) number.

But here, we do not speak about the SVG tag but the UX:Icon component instead. Which IMO should behave like other TwigComponent and correctly auto-cast int/string values, ex:

#[AsTwigComponent]
final class AutocastInt
{
    public string $min;
    public int $max;
}
<twig:AutocastInt min="1" max="10" />
<twig:AutocastInt :min="1" :max="10" />
<twig:AutocastInt min="{{ 1 }}" max="{{ 10 }}" />

which renders as expected:
image

Like in Vue or React/JSX, the following codes are correctly interpreted:

<input type="range" :min="1" max="120" />
<input type="range" min="1" max={10} />

Anyway, I don't really understand why do we need such this validation, can't we rely on ComponentAttributes and let it deal with attributes rendering?

@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Sep 12, 2024
@smnandre
Copy link
Collaborator

But here, we do not speak about the SVG tag but the UX:Icon component instead. Which IMO should behave like other TwigComponent and correctly auto-cast int/string values, ex:

This is not directly related to the UX:Icon component (that does not really exist, in fact 😓 ) .. so we can either force the string in the Listener, in the Renderer, or ... in the Icon as you did.

But i don't want to store int as "int", but as strings in Icon attributes property.. so what do you prefer ?

@smnandre
Copy link
Collaborator

As i said, I agree to cast it, just not internally :)

src/Icons/CHANGELOG.md Outdated Show resolved Hide resolved
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Sep 12, 2024
@smnandre
Copy link
Collaborator

Thanks for fixing this bug Hugo.

@smnandre smnandre merged commit 781f66c into symfony:2.x Sep 12, 2024
20 checks passed
@Kocal Kocal deleted the 2148-ux-icon-attr-int-float branch September 12, 2024 18:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug Bug Fix Icons Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TwigComponent][Icons] Inconsistency between allowed attributes
3 participants