-
-
Notifications
You must be signed in to change notification settings - Fork 1k
chore: Improve launcher framework combobox (#2895) #2899
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
1. The reason this line is barely visible is because it's the default setting when IsEnabled is set to false. The easiest way to fix this is to change the check condition from 1 to 0. 2. I changed the version description from .NET Core XX to .NET XX. Because it says "It isn't .NET Core anymore, it should be just .NET 10 or .NET 8," I also deleted an if branch.
@dotnet-policy-service agree |
Thanks @AIFSKATE , visually it looks definitelly better. I will let others to review your updates. |
@VaclavElias was your intent to have the combo box be interactible when there is just one framework to choose from, or keep it non-interactible but slightly more visible than it currently is ? I think keeping it non-interactive is better as it conveys to the user that there is nothing for them to change there. Otherwise users might press on the combo box just to check if there is anything else they could set it to when that box doesn't have anything else but the default value. |
Pre net6.0 I think ? The launcher should support every engine version as long as feasible, so best leave that combo box in and have it slightly more opaque if possible. |
I see, so the reason it wasn't visible was because there was only 1 item. What about, if there is only 1 item, it should be only a text? |
if (framework.Framework == ".NETFramework") | ||
return $".NET {framework.Version.ToString(3)}"; | ||
else if (framework.Framework == ".NETCoreApp") | ||
return $".NET Core {framework.Version.ToString(2)}"; | ||
if (framework.Framework == ".NETCoreApp") | ||
return $".NET {framework.Version.ToString(2)}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required for older versions that supported multiple frameworks.
</Button> | ||
</Grid> | ||
<ComboBox Margin="0,4" Name="FrameworkSelector" IsEnabled="{Binding ActiveVersion.Frameworks.Count, Converter={sskk:Chained {sskk:IsGreater}, Parameter1={sskk:Double 1}}}" ItemsSource="{Binding ActiveVersion.Frameworks}" SelectedItem="{Binding ActiveVersion.SelectedFramework}" SelectionChanged="FrameworkChanged"> | ||
<ComboBox Margin="0,4" Name="FrameworkSelector" IsEnabled="{Binding ActiveVersion.Frameworks.Count, Converter={sskk:Chained {sskk:IsGreater}, Parameter1={sskk:Double 0}}}" ItemsSource="{Binding ActiveVersion.Frameworks}" SelectedItem="{Binding ActiveVersion.SelectedFramework}" SelectionChanged="FrameworkChanged"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's kind of a useless check since it can never be negative. The reason it was greyed out was because there is no need to select anything when only one framework is available.
I'd rather not display anything in that case, i.e. change IsEnabled
to IsVisible
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we display plain text (in white) when there's only one item? It would still be helpful to show the .NET version so that those unfamiliar with Stride can see it.
The behavior would remain the same when multiple items are available, but if there's only one item, the combo box wouldn't be displayed, just plain white text instead.
PR Details
Related Issue
#2895
Types of changes
Checklist