-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add combobox UI #263
Add combobox UI #263
Conversation
|
Agree with @simurai on the border. The assignee dropdown has no border, only the checkbox, should probably make both dropdowns consistent? EDIT: And by border I mean the border + darker background. |
| <ui:FilterTextBox | ||
| x:Name="filterAssignee" | ||
| Grid.Row="0" | ||
| Foreground="{DynamicResource GitHubVsWindowText}" |
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.
@shana I was hoping to be able to move this to the style declarations around line 62 but it doesn't seem to set the foreground in FilterTextBox.xaml.
Any insights as to why this isn't happening?
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.
Hmmm, no clue 😕
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.
I haz clue! Take out https://github.com/github/VisualStudio/blob/add-combobox-ui/src/GitHub.UI/Assets/Controls/FilterTextBox.xaml#L126, it's resetting the foreground in such a way that it overrides dynamic resource'd settings.
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 did it! ✨
Thanks for looking into it!
since it doesn't work
438adb8 to
3867ac2
Compare
|
Added themes for the combo box. The light and blue themes share the same colors while the dark theme is it's own thing. There's an issue where the prompt TextBox text is vertically off-centered which I plan on fixing next: Would still love some 💭 and 👀 @shana This UI be moved to |
Let's put it in GitHub.VisualStudio.UI. That's going to require moving the style files as well, so just put the style in its own file in the Styles directory, and we'll move all of it later. |
👍 Excellent, I'll go ahead and do that. |
| <Setter Property="Foreground" Value="{DynamicResource GitHubVsToolWindowText}" /> | ||
| </Style> | ||
|
|
||
| <Style TargetType="{x:Type ui:OcticonImage}" BasedOn="{StaticResource {x:Type ui:OcticonImage}}"> |
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 line triggers a XAML parse error:
{"Cannot find resource named 'GitHub.UI.OcticonImage'. Resource names are case sensitive."}
I'm assuming I'm not resourcing correctly in Visual Studio on Line 3 😛
<ResourceDictionary xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:ui="clr-namespace:GitHub.UI;assembly=GitHub.UI">Any chance of pointing me in the right direction of understanding how to get the right resource? I've been trying to follow how we're doing it in PullRequestCreationView.xaml.
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.
Hmmm, the xmlns:ui declaration is correct, the OcticonImage type is in the GitHub.UI namespace as declared here: https://github.com/github/VisualStudio/blob/master/src/GitHub.UI/Controls/Octicons/OcticonImage.cs#L5.
It might need the GitHubUI SharedDictionary.xaml file where the OcticonImage.xaml is included from (where the style is eventually declared).
Try adding to the top:
<ResourceDictionary.MergedDictionaries>
<cache:SharedDictionaryManager Source="pack://application:,,,/GitHub.UI;component/SharedDictionary.xaml" />
</ResourceDictionary.MergedDictionaries>
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 did it! Added in 7a334de
Make it look a little more centered if it's a single line text field
| </Border> | ||
|
|
||
| <Grid Margin="1,0,0,0"> | ||
| <Grid Margin="1,2,0,0"> |
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.
Does this affect the UI in the Login/RepositoryCreation/RepositoryClone controls? Are they using this as well?
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 a good question! I'll double check right now
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.
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.
purrrrfect!
|
@donokuda I added a comment about the margin changes you did. Otherwise, things look ✨ ! I'll do a final pass and merge if you're happy with this branch 😄 |









Destination branch:
don/stub-uiThis PR stubs out a filterable combobox ui for selecting branches and assignees. I originally went with a UI very similar to what we have in Desktop; however, it felt out of place with Visual Studio's UI. I ended up trying out a different approach with a look and feel closer to a native context menu.
Desktopish (Mock-up)
(I have a branch with this style but it has become so stale that it won't build on my machine without a bit of troubleshooting 😭)
Context menu-ish

There's still some refinement, polish, and theming, that needs to be done; but I would love some thoughts on this direction.
/cc @shana, @simurai, @niik for 👀 and 💭 (although feedback from anyone is encouraged)