Polish peek view for inline reviews #1080
Conversation
…ui/margin-polish
This file can contain design time values for any resources that are defined at run time.
It no longer sets the background color explicitly in the glyph factory.
This will be refreshed when the theme changes.
The following application resources will be available: pack://application:,,,/GitHub.UI;component/SharedDictionary.xaml pack://application:,,,/GitHub.UI.Reactive;component/SharedDictionary.xaml pack://application:,,,/GitHub.VisualStudio.UI;component/SharedDictionary.xaml
| <Style TargetType="UserControl"> | ||
| <Style.Triggers> | ||
| <DataTrigger Binding="{Binding DiffChangeType}" Value="Add"> | ||
| <Setter Property="Background" Value="{DynamicResource DiffChangeBackground.Add}" /> |
There was a problem hiding this comment.
I've created 3 new dynamic brushes that will change according the the theme. They're called DiffChangeBackground.Add, DiffChangeBackground.Delete and DiffChangeBackground.None to mirror the DiffChangeType enum.
The old DiffChangeBackground is no longer being set for each glyph. Everything is now done via the DiffChangeType property.
|
I noticed the arrows won't showing up here: Strange, because they show in your screen grabs. I did merge from |
|
|
||
| <UserControl.ToolTip> | ||
| <ToolTip | ||
| Background="{DynamicResource GitHubVsToolWindowBackground}" |
There was a problem hiding this comment.
You can now access the GitHub dynamic/themed resources from the glyph XAML files (this wouldn't have worked before). You can see I'm using GitHubVsToolWindowBackground here.
| xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"> | ||
|
|
||
| <SolidColorBrush x:Key="DiffChangeBackground.Add" Color="Green" /> |
There was a problem hiding this comment.
If you want any resources to show up in the designer, you can add there here.
You can see below, I'm also adding the 3 SharedDictionary.xaml files. At the moment this is only needed for the Show/AddInlineCommentGlyph.xaml files (because they're added by BrushesManager.cs rather then an explicit MergedDictionaries element in each XAML file).
Removed need for `BrushesManager`.
🤔 Hmm, that is strange.. I'll take a lookie loo at this. |
|
Hmm, not running into that problem here... (EDIT: Although the timestamp color needs fixing)
|
|
@donokuda master merged in. this is how it looks to me right now:
|
|
@shana Thank you! 🙇 Looks like there's some styling stuff I need to fix but it should be pretty straightforward
Or at least more straightforward than me attempting to fix merge conflicts 😄 |
|
Just taking a look at this now: looks great in general, a few things though:
|
Conflicts: src/GitHub.InlineReviews/Views/InlineCommentPeekView.xaml
Good calls on both. For the textbox, I updated to use the searchbox background color that VS provides us. Here's what the peek view looks like right now:
|
Updated the margins:
|
|
Updated codeblock styles:
@grokys @jcansdale I think this is ready for another review. This branch now has better support for themes and fixes up the peek view so that the experience is better than it was before (by removing and reduce the sizes of some elements.) |
| <Border Background="{DynamicResource GitHubGlyphMarginCommentableBackground}" BorderThickness="0,0,1,0" /> | ||
| <Viewbox x:Name="AddViewbox" Margin="1,1,0,0"> | ||
| <Canvas Width="14" Height="14"> | ||
| <Rectangle Width="13" Height="13" /> |
There was a problem hiding this comment.
Yup! The background is being set with a style.
|
|
||
| <Canvas.Resources> | ||
| <Style TargetType="Rectangle"> | ||
| <Style.Triggers> |
There was a problem hiding this comment.
Are these triggers still used? AFAICS the add glyph is now always the same color?
| <Canvas.Resources> | ||
| <Style TargetType="Path"> | ||
| <Style.Triggers> | ||
| <DataTrigger Binding="{Binding DiffChangeType}" Value="Add"> |
There was a problem hiding this comment.
Again, are these triggers needed?
| --> | ||
|
|
||
| </Grid.Background> | ||
| <Border Background="{DynamicResource GitHubPeekViewBackground}" BorderBrush="{DynamicResource GitHubPeekViewBackground}" BorderThickness="0,0,1,0"/> |
There was a problem hiding this comment.
Do we need to specify a border when the background is the same color?
There was a problem hiding this comment.
Hmm, maybe not. I'll check on this.
|
Here's what I got so far as of 2ba07ba. Good call on giving a little more spacing between the author information and the comment.
|
|
@grokys Ready for another 👀 |
This pull request starts bringing in support for themes to inline reviews.
Doesn't look 100% great so I'll be following up with some custom color choices in a series of future pull requests. Here's what it looks like so far:
Light
Dark
Blue
