Fit icon to size instead of static size

[sn4k3]

Problem

Icons have static size, currently 24x24.
By default, this is not a good design, because some elements may grow and outscale the text, for example MenuItem or small buttons and potential break the design.
We can of course use classes and/or manual size, but defining this every time is ugly, when should be the other way around (Only define it where needed).
Most of times we use icons inside constrained containers, eg: Buttons, Menus, TextBlock and other controls that already define their height by the content.

See this example (3 buttons with a StackPanel[Icon Text]):

To match text I would have to either have classes or manual set size for each icon (Ugly by default)

Solution

Size it to the parent FontSize it's IMO the most correct approach, other icon libraries do that and I never encounter a problem nor the need of set any of the icons with manual size. And to be correct in design terms we want to align icons with text.

The implementation would translate to:

<Viewbox Width="{TemplateBinding FontSize}"
         Height="{TemplateBinding FontSize}">
  <Path Data="{TemplateBinding Kind, Converter={StaticResource GeometryConverter}}"
             Fill="{TemplateBinding Foreground}" />
</Viewbox>

Results in:

So it now respects parent FontSize (12 / 15 / 28, in this example) and Icon integrates much better.

For the rare cases we want to override this, we pass the FontSize for Icon.

I can submit a PR but would like to ask first. As this is a breaking change. Height and Width would no longer exists in favor of FontSize, just like MaterialIconExt does...

[SKProCH]

Hmm, its actually interesting idea, i like it. Be we can't remove Height and Width since it avalonia control base properties.

[SKProCH]

Use only FontSize as the effective icon size (Like other libraries do)

Did you mean use the TextElement.FontSize property (without creating anything)?

If yes, i can't agree that a more cleaner solution, since user should every time write **TextElement.**FontSize to set icon size. I think this is not very logical.

The 2. option can "simplify" and avoid confusion where MaterialIcon would use FontSize and MaterialIconText uses IconSize and MaterialIconExt uses Size (3 different properties names in the ecosystem).

Seems like i kinda missed the bad naming for IconSize. As we doing the breaking change anyway, probably we can also rename this to Size to unify all the names across the library?

If we rename everything to Size we can use TextElement.FontSize as a text size control for a MaterialIconText. So Size controls the icon size everywhere, and TextElement.FontSize control text size (as stated in property class and property name). And by default icon size will follow the FontSize.

---

we need to override and implement a kind of redirect

About this condition, I don't see a correct way to implement this...

No, no need to use such code. Actually this can be done via styles. Like:

<Style Selector="^:not(^[Size=NaN]) /template/ Viewbox">
  <Setter Property="Height" Value="{Binding $parent[icon:MaterialIcon].Size}" />
  <Setter Property="Width" Value="{Binding $parent[icon:MaterialIcon].Size}" />
</Style>
<Style Selector="^[Width=NaN][Height=NaN][Size=NaN] /template/ Viewbox">
  <Setter Property="Height" Value="{TemplateBinding TextElement.FontSize}" />
  <Setter Property="Width" Value="{TemplateBinding TextElement.FontSize}" />
</Style>

And here it is (from 1-4 my points)

In WPF/WinUI i suppose this can be done via DataTriggers.

In the end will be a mess and width/height should always be the container size and not its child size.

Yeah, its exactly what it is - Width/Height controls the containers size, child size controlled by Size. If not specified, fill the full container using Width/Height, if not specified - FontSize.

But, i have to agree with you, this logic If not specified, fill the full container using Width/Height, if not specified - FontSize. kinda ugly.
From the other hand, if I, as a user, specify the Width/Height (without Size) i probably want icon to fill all container, so probably this is isn't a bad idea and makes the user experience better imo. (and also removes requiring to refactor old code)

I still want to hear your opinion on both of these questions. We can also release a prerelease (e.g. with my implementation as the most complex one) and host a poll/listen to a feedback.

[sn4k3]

Did you mean use the TextElement.FontSize property (without creating anything)?
If yes, i can't agree that a more cleaner solution, since user should every time write **TextElement.**FontSize to set icon size. I think this is not very logical.

Yes, But why the requirement of TextElement? It's is not required, at least in Avalonia. You simply write FontSize=16 no need to prepend the ugly TextElement. This is true for all other controls inside avalonia. (I don't know about WPF or WinUI)

Example of Projektanker.Icons library implementation (FontSize):

https://github.com/Projektanker/Icons.Avalonia/blob/82db70624c3f36079f09d3305a3e30c4fffae446/src/Projektanker.Icons.Avalonia/Icon.axaml#L42-L46

Seems like i kinda missed the bad naming for IconSize

IconSize name was my implementation. I opted to name it IconSize to make sense for human reading and pairing with Icon. As you have Text and Icon property, with a single Size property, what does it mean? Will it affect Icon? Text? Both? IconSize offers a clear indicator that will only affect Icon size.

As we doing the breaking change anyway, probably we can also rename this to Size to unify all the names across the library?

We can, or offer extra property like TextSize and keep the IconSize, I'm not sure here...
A breaking change on properties are non-problem since compiler won't let you go further, so user have no chance other than procede with refactoring and accept the change.

No, no need to use such code. Actually this can be done via styles. Like:

Yes, I was more concerned with WPF and WinUI as I know they don't have that powerful styling as Avalonia. But I will leave the WPF and WinUI implementation for you as I never used them...

And here it is (from 1-4 my points)

I still want to hear your opinion on both of these questions. We can also release a prerelease (e.g. with my implementation as the most complex one) and host a poll/listen to a feedback.

As you might, I'm also puzzled in what should be better in this situation...
For the sake of simplicity my head tell me to drop this complex logic and use the inbuilt FontSize and handle icons like fonts.
On the other hand your suggestion makes sense and saves users from potential break change and/or mistakes.
For future users none of this seems relevant.

[SKProCH]

Yes, But why the requirement of TextElement?

Really? Wow, i don't know that they added TextElement properties to every TemplatedControl... Thanks for sharing this.

I opted to name it IconSize to make sense for human reading and pairing with Icon

Yeah, that makes sense. We can use IconSize everywhere, actually. While leaving the FontSize for font only and default icon size.

This will make the most sense, IconSize for icon, FontSize for text.

But I will leave the WPF and WinUI implementation for you as I never used them...

Yes, leave it to me, i'll try to do that. If i fail we can always rollback to your simple proposal

---

So, lets stick with unifying the IconSize and complex implementation (i handle the WPF/WinUI part)?

[sn4k3]

Really? Wow, i don't know that they added TextElement properties to every TemplatedControl... Thanks for sharing this.

It this something avalonia or wpf used before?
Because Avalonia still does TextElement but that's only if you don't want to apply any of it properties to the control itself. This way you can use TextElement in a StackPanel to define potential targets inside it, but not StackPanel itself.

So, lets stick with unifying the IconSize and complex implementation (i handle the WPF/WinUI part)?

Sure, let me prepare the PR. I have many things coming :)

[SKProCH]

It this something avalonia or wpf used before?
Because Avalonia still does TextElement but that's only if you don't want to apply any of it properties to the control itself. This way you can use TextElement in a StackPanel to define potential targets inside it, but not StackPanel itself.

It actually just register the same property for TemplatedControl. So, actually FontSize and TextElement.FontSize should have the same effect, since it actually the same property.

not StackPanel itself

Cuz the StackPanel isn't the TemplatedControl, it doesn't have this property. It can be specified for non templated controls via class name.