Ironsides - Tooltips for the bar items #3107
Conversation
| <TextBlock x:Uid="AppPID" Text="{x:Bind _viewModel.ApplicationPid, Mode=OneWay}" FontFamily="Segoe UI" FontSize="10" Margin="5" VerticalAlignment="Center" Visibility="{x:Bind _viewModel.AppBarVisibility, Mode=OneWay}"/> | ||
| <TextBlock x:Uid="AppCPUUsage" Text="{x:Bind _viewModel.AppCpuUsage, Mode=OneWay}" FontFamily="Segoe UI" FontSize="10" Margin="5" VerticalAlignment="Center" Visibility="{x:Bind _viewModel.AppBarVisibility, Mode=OneWay}"/> | ||
|
|
||
| <!-- Per System controls --> | ||
| <Button x:Uid="SwitchLayoutButton" x:Name="SwitchLayoutButton" HorizontalAlignment="Center" Click="{x:Bind _viewModel.SwitchLayoutCommand}"> | ||
| <TextBlock Text=""/> | ||
| </Button> | ||
| <Button x:Name="LargeContentButton" Click="{x:Bind _viewModel.ShowBigWindowCommand}" HorizontalAlignment="Center"> | ||
| <Button x:Name="LargeContentButton" x:Uid="LargeContentButton" Click="{x:Bind _viewModel.ShowBigWindowCommand}" HorizontalAlignment="Center"> |
There was a problem hiding this comment.
NIT: a more descriptive Id. "LargeContentButton" is ambiguous. #Resolved
| @@ -68,15 +70,15 @@ | |||
| </controls:ProcessSelectionButton> | |||
|
|
|||
| <!-- Per App controls --> | |||
| <Image x:Name="AppIcon" HorizontalAlignment="Center" Source="{x:Bind _viewModel.ApplicationIcon, Mode=OneWay}" Margin="5" Height="20" Width="20" Visibility="{x:Bind _viewModel.AppBarVisibility, Mode=OneWay}"/> | |||
| <Image x:Name="AppIcon" HorizontalAlignment="Center" Source="{x:Bind _viewModel.ApplicationIcon, Mode=OneWay}" ToolTipService.ToolTip="{x:Bind _viewModel.ApplicationName, Mode=OneWay}" Margin="5" Height="20" Width="20" Visibility="{x:Bind _viewModel.AppBarVisibility, Mode=OneWay}"/> | |||
There was a problem hiding this comment.
Please put each property on their own line. #WontFix
There was a problem hiding this comment.
Just my 2cents... obvious consistency is important, but my vote would be to have all the properties on one line. When I'm trying to see how all of my controls fit into a layout on the page, having a line per control is more readable (to me) than having a single control take up a quarter of the page. I think this borders on personal preference though...
There was a problem hiding this comment.
Just to add another two cents🙂... generally, we put everything on one line when it's short enough to fit and split when it's not. Personally, I find that easier to read in an IDE, but I also think splitting long lines makes it easier to review on GitHub. I'm fine with the wontfix, just wanted to add a little context.
| @@ -51,6 +51,9 @@ public partial class BarWindowViewModel : ObservableObject | |||
| [ObservableProperty] | |||
| private Visibility _appBarVisibility = Visibility.Visible; | |||
|
|
|||
| [ObservableProperty] | |||
| private string? _applicationName; | |||
There was a problem hiding this comment.
NIT: remove ? and assign string.empty in the constructor. #Resolved
There was a problem hiding this comment.
Ok. Seems to fit the pattern better for the rest of the file. I'm guessing a tooltip of null is different than a tooltip of "", but in theory the user won't have access to the tooltip of an app if we're not attached to an app.
Summary of the pull request
References and relevant issues
Adds tooltips for the bar items that didn't have them.
Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist