Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
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}" /> |
jcansdale
Jul 20, 2017
Collaborator
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'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}" |
jcansdale
Jul 20, 2017
Collaborator
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.
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" /> |
jcansdale
Jul 20, 2017
Collaborator
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).
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, not running into that problem here... (EDIT: Although the timestamp color needs fixing)
|
|
@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.) |
|
A few comments on possibly unneeded code. Also do you think we could afford a little bit of vertical space between the inline comment header and body? They look just too close together now:
|
| <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" /> |
grokys
Aug 8, 2017
Contributor
Is this Rectangle needed?
Is this Rectangle needed?
donokuda
Aug 8, 2017
Author
Member
Yup! The background is being set with a style.
Yup! The background is being set with a style.
|
|
||
| <Canvas.Resources> | ||
| <Style TargetType="Rectangle"> | ||
| <Style.Triggers> |
grokys
Aug 8, 2017
Contributor
Are these triggers still used? AFAICS the add glyph is now always the same color?
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"> |
grokys
Aug 8, 2017
Contributor
Again, are these triggers needed?
Again, are these triggers needed?
| --> | ||
|
|
||
| </Grid.Background> | ||
| <Border Background="{DynamicResource GitHubPeekViewBackground}" BorderBrush="{DynamicResource GitHubPeekViewBackground}" BorderThickness="0,0,1,0"/> |
grokys
Aug 8, 2017
Contributor
Do we need to specify a border when the background is the same color?
Do we need to specify a border when the background is the same color?
donokuda
Aug 8, 2017
Author
Member
Hmm, maybe not. I'll check on this.
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 |
|
LGTM now! |
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
