hlm-icon: Changing the [size] binding dynamically does not resize icon in layout.

[Londovir]

Please provide the environment you discovered this bug in.

Minimal reproduction is located here: Stackblitz recreation

Which area/package is the issue in?

Don't know / other

Description

In the HlmIconComponent, used for displaying icons, if you dynamically bind the [size] property, and then change the size after the initial rendering, the new size is not reflected in the layout. It appears that the _computedClass computed() is only removing the TailwindCSS applied width and height styling if the variant is set to 'none'; however, when you use dynamic binding such as in my reproduction, then the internal signal _hostClasses is getting set on initial rendering to the original TailwindCSS classes needed for the initial sizing, and then any changes afterwards are essentially ignored.

For example, rendering with an initial size of 'base' will setup 'h-6 w-6' into _hostClasses. If you then use radio buttons to change the sizing to 'xs', the _computedClass process will skip the removal of the TailwindCSS classes (since the variant will not be 'none', and the final call to hlm() will cause the variant's classes to be overridden at the end by the existing _hostClasses signal value.

I have tentatively fixed this on my codebase by changing the _computedClass process to always run the replacement line, regardless of the variant value. This seems to properly remove the h and w classes that may already be set in _hostClasses, and since the 'none' variant adds an empty string of CSS clases, this preserves the overall desired result.

Please provide the exception or error you saw

Sizing classes from TailwindCSS applied by the component are not updated correctly when the [size] property is updated dynamically after initial rendering.

Other information

No response

I would be willing to submit a PR to fix this issue

• [X] Yes
• [ ] No

[goetzrobin]

@elite-benni I know you worked on the Icon component and the stackblitz actually has a fix in it. Do you want to take a look at this?

[elite-benni]

@goetzrobin It has something to do with your change on my Accordion Pull request. https://github.com/goetzrobin/spartan/pull/64/commits/edc217e6da7f49b8e7e0dd87053dda4e2f68fab6

Can you remember wich directive was setting classes or how to test it with this solution?

The problem seems to be, that the MutationObserver also saves the classes, that are set by the variant. After that, this size(h and w) is in all variants because it is the last param in hlm() and overwrites the variant size.

For my understanding, the fix should be, removing tailwind H and W when the variant is set, instead of only removing it if it is none. in this case, a directive can set H and W for none Variants. If a size variant is set, H and W should be removed, because size is set by the variant.

So the ternary should be reversed.

const hostClasses = variant === 'none' ? this._hostClasses().replace(TAILWIND_H_W_PATTERN, '') : this._hostClasses() ;

should be ...

const hostClasses = variant === 'none' ? this._hostClasses() : this._hostClasses().replace(TAILWIND_H_W_PATTERN, '') ;

What do you think?

[goetzrobin]

Makes perfect sense to me! I'm wondering what did make me make that change... It might have been the accordion icon directive 🧐

[goetzrobin]

Will be fixed by: https://github.com/goetzrobin/spartan/commit/d9144da724ac840ed100c83526bacaca94b55355

[goetzrobin]

@Londovir this fix was released with the newest alpha. Let me know if this works after upgrading!

[goetzrobin]

Released as part of the newest alpha!