Skip to content
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

Confusing attributes #239

Open
Tracked by #265
MartinZikmund opened this issue Apr 15, 2024 · 4 comments
Open
Tracked by #265

Confusing attributes #239

MartinZikmund opened this issue Apr 15, 2024 · 4 comments
Assignees
Labels
kind/consumer-experience Categorizes issue or PR as related to improving the experience of consumers kind/enhancement New feature or request. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@MartinZikmund
Copy link
Member

MartinZikmund commented Apr 15, 2024

What would you like to be added:

UnoIcon consists of Include and ForegroundFile - which together compose the icon. However, technically the Include is the "background" and the actual icon is the ForegroundFile - this is quite weird. In case of splash screen, the Include is the actual icon.

When the user, who is not familiar with it comes in, they will most likely replace icon.svg with their icon and run the app - which will cause it to be their icon in the background and uno logo in front of it...

I think it would be more meaningful if the Include were the actual icon and there was a BackgroundFile attribute instead. The files also should be named more clearly icon_background.svg and icon.svg.

Similarly, I feel that Color attribute could become BackgroundColor, as now its purpose is not clear.

Why is this needed:

Clearity.

@MartinZikmund MartinZikmund added kind/enhancement New feature or request. triage/untriaged Indicates an issue requires triaging or verification. labels Apr 15, 2024
@pedrojesus-work
Copy link
Contributor

MSBuild properties have this limitation, where Items outside target should have one of those operations: Include, Update or Remove. With that we use the Include to specify the asset that will be used.

If the user gets confuse, it can look into docs where he/she can see all the properties that are available and its meaning.
image

@MartinZikmund
Copy link
Member Author

In such case still it would probably feel more meaningful if the Include was the icon itself and there was a IconBackground attribute for its background for example. However, I understand this technically a breaking change...

@dansiegel
Copy link
Contributor

dansiegel commented May 30, 2024

Proposal:

  • *LEGACY SUPPORT* When UnoIcon has both Include and ForegroundFile we will process this as we always have.

  • When we have UnoIcon with no ForegroundFile we will treat this as the actual icon and use a transparent background if one is not provided.

  • When we have a UnoIconBackground we will apply this as the background file to the UnoIcon. This should be optional as many times you really just want a color and the icon.
    NOTE: We would want this as a new Item rather than in an Attribute on the UnoIcon because this will help MSBuild better watch the file for changes. Currently this is problematic for Resizetizer because if you change the foreground file MSBuild has no way of knowing it changed.

As @MartinZikmund suggested we can add a BackgroundColor attribute which can set the Color property in the MSBuild task. This would not have any backwards compatibility issues so long as we set the Color from the BackgroundColor.

@dansiegel dansiegel added kind/consumer-experience Categorizes issue or PR as related to improving the experience of consumers priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed triage/untriaged Indicates an issue requires triaging or verification. labels May 30, 2024
@MartinZikmund
Copy link
Member Author

This set of changes sounds good to me 🚀 ! For the ForegroundScale I guess can then be "replaced" by Scale as well (as it will be on UnoIcon only anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/consumer-experience Categorizes issue or PR as related to improving the experience of consumers kind/enhancement New feature or request. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

3 participants