-
Notifications
You must be signed in to change notification settings - Fork 21
Adds Invisible and Transparent icons #61
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
base: master
Are you sure you want to change the base?
Conversation
# Adds special icons: - **Invisible**: Do not display the icon, and do not reserve space for it in layout. - **Transparent**: Do not display the icon, but reserve space for the element in layout. (Default icon) The Transparent is now the default icon since common use is to have element size untouch when change icon. Fixes SKProCH#58 # Other fixes - Fix the lack of IconSize property on avalonia design time, causing error due two constructors and taking IconAnimation as precedence. - Fix inproper use of MaterialIconExt as control in avalonia design time
|
I thought about this issue too I think that we just need to make the Kind nullable, this is perfectly handles the case with "transparent" default icon, while also making user's life easier to just pass null instead of making specific branching logic/converters. And for "invisible" user can just utilize the What do you think? |
|
I also tought about going nullable, but I went this road because offers more flexibility and I don't like nullable enums 😅 It's more common pratice to have Invalid/None/Empty as first enum value than using null enums. With this is route user can:
Code-wise, the coverage for null will be more "stressful" I'm tagging @fls-eugene as he raised the issue, what is your preference? |
|
Yeah, I can understand your position
With a nullable approach users can achieve the same.
But we blurring responsibility here I think. Currently enum contains only actual icons, without leaking the viewing layer "variants" kinda. Also, we adding the styles (or triggers) check to all icons instances only to handle a very specific rare, almost nonexistent usecase. Yes, this is not a something very compute intensive, but should we really waste compute to handle such rare case? This is a corner case which behaves differently from all other icons. Also we need to think what we should do with a MaterialIconTextExt with Invisible/Transparent icon. I'm not sure that we actually just need to hide only the icon (and keep text visible). Not handling this in a such way allows us to not bother with such things.
Nah, the nullable reference types handles it fine I suppose Tbh, currently I still prefer the nullable approach, but I've open to discussion. Let's wait a little and see if somebody (fls-eugene?) writes something about this |
|
Maybe we can commit a null branch and compare?
I think it only should apply to icon as we are dealing with Icon properties anyway. |
|
In
The community has grown accustomed to these standard Moreover, introducing Overall, adding functionality that is already conveniently achievable via standard means is strongly discouraged. The above concerns equally apply to the nullable approach as well. |
Alternative to SKProCH#61 - Allow `Kind` to be `Nullable` (Default: null) - Add `NullAsInvisible` class # Other fixes - Fix the lack of IconSize property on avalonia design time, causing error due two constructors and taking IconAnimation as precedence. - Fix inproper use of MaterialIconExt as control in avalonia design time
Adds special icons:
The Transparent is now the default icon since common use is to have element size untouch when change icon.
Other fixes
Please review: