search

davidxuang / FluentIcons

A multi-framework wrapper of https://github.com/microsoft/fluentui-system-icons
MIT License
72 stars 6 forks source link

Recent base class change from Avalonia IconElement to Control #6

Closed StefanKoell closed 4 months ago

StefanKoell commented 5 months ago

Hi!

I noticed that since this change https://github.com/davidxuang/FluentIcons/commit/80ae5f284745f85d569bf372712f372d1b973689# the base class for SymbolIcon has changed to Control (from IconElement). I'm wondering why that was changed? Any insights?

Regards, Stefan

davidxuang commented 5 months ago

Hi,

This change is because that this class never implements border-related properties of TemplatedControl (which is the base class of IconElement), and BorderRenderHelper from Avalonia is only visible internally. And the initial reason for this is to be aligned with other XAML frameworks.

Since Avalonia now features PathIcon as the first official implementation of IconElement, do you think it's better to implement border-related properties instead?

StefanKoell commented 5 months ago

To be honest, I'm not sure what would be the better approach. It's just that I used IconElement for a lot of things and this update broke almost all my references in various view models. It's an easy fix to change all to Control but I thought it was nice to have the common base class IconElement to have a bit of a constraint (and semantic hint) what's to expect. For a minor version update I thought it was quite a breaking change.

I've seen that the Fluent Avalonia variant (derived from FAIconElement) is still using the IconElement inheritance chain. Really not sure what's the better apprach.

davidxuang commented 5 months ago

I've just revoked the base class change and implemented Background, BorderBrush, BorderThickness, CornerRadius and Padding by hosting a Border instance. Still, there is a sematic problem that this TemplatedControl is not actually implemented by a ControlTemplate.

StefanKoell commented 5 months ago

Hmm, since you do the rendering directly in code, there's no need for a template, right? I'm also not completely sure if that's just a cosmetic issue or if has other implications.

davidxuang commented 5 months ago

Never tried this but Template is exposed as a property, which seems weird for icon controls. I'm wondering why Avalonia chose so.

maxkatz6 commented 5 months ago

Because in Avalonia these are templated controls, which do not render themselves. From PathIcon code: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Themes.Fluent/Controls/PathIcon.xaml#L17-L25

davidxuang commented 4 months ago

I'm closing this as relevant changes have been revoked now. Feel free to reopen if there are more problems.