-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Misc visual fixes #12916
Misc visual fixes #12916
Conversation
work in progress comparison shots:
|
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.
Okay not totally done reviewing yet. Most of this looks really good. Things I'm worried about:
- The crash when selecting a color via the color picker. That's gotta get fixed. I don't have a beat on the stack yet.
- I personally don't love the changes to the New Tab button. I kinda liked that it felt like it was another tab. Now it kinda feels a little too squashed.
Also tapping in our design captain @cinnamon-msft for additional feedback
@@ -43,5 +43,12 @@ | |||
<local:MinMaxCloseControl x:Name="MinMaxCloseControl" | |||
Grid.Column="2" | |||
HorizontalAlignment="Right" /> | |||
|
|||
<!-- This border needs to be added manually until GH#10320 is fixed --> |
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 would almost reckon that this fixes #10320. From an architectural standpoint, I think it's gonna be Hard to make the TabView actually span the whole width, but if this fakes it out, then I'm cool with that as the resolution here.
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.
Couldn't the drag bar and caption buttons simply be moved to be inside the TabView footer? Currently they're sitting right next to it, and it seems like it would be easy.
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.
Somewhat -- but right now there's an ownership inversion between the titlebar control and the tabbar control. The title bar contains [tabview content zone, dragbar, caption buttons]
, and they're three distinct areas because when "show tabs in the titlebar" is turned off it doesn't exist and the tabview is kept in the page root.
We'd need to flip around the ownership of the drag bar and caption buttons and pull them out into the titlebar control when "show tabs in the titlebar" was turned on, and otherwise make sure that they are not present when "show tabs in titlebar" is off.
void ColorPickupFlyout::ColorButton_Click(IInspectable const& sender, Windows::UI::Xaml::RoutedEventArgs const&)
{
auto button{ sender.as<Windows::UI::Xaml::Controls::Button>() };
auto rectangle{ button.Content().as<Windows::UI::Xaml::Shapes::Rectangle>() };
auto rectClr{ rectangle.Fill().as<Windows::UI::Xaml::Media::SolidColorBrush>() };
_ColorSelectedHandlers(rectClr.Color());
Hide();
} Well yep that'll do it |
Color buttons now have a colored background and an overlay with the default button style, so that they change on hover/press. |
I think these changes look good and they align with our design guidelines (from what I can tell). I'm wondering if we just make the split button a bit taller to match the height of the tabs? That may help the squishiness. |
Definitely. I think we're cool with that. I discussed more with the team and everyone else was cool with this sort of design. They definitely turned me around on it too With that fix to the new tab button, the only blocking bit would be to get that color picker crash sorted. Lemme see if I can't whip a diff up quick. |
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.
Hello, I am the repairaccount.
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.
access
This comment was marked as off-topic.
This comment was marked as off-topic.
@Shomnipotence We're tracking that kind of redesign more broadly in #3327 (and in the spec over at #12530). This PR's more focused on some smaller scale polish, rather than a broader redesign. |
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.
Thanks for putting these all together. Played with the latest commit and it feels good.
@@ -351,11 +352,9 @@ | |||
<ControlTemplate x:Key="VerticalThumbTemplate" | |||
TargetType="Thumb"> | |||
<Rectangle x:Name="ThumbVisual" | |||
contract7NotPresent:RadiusX="{Binding Source={ThemeResource ScrollBarCornerRadius}, Converter={StaticResource TopLeftCornerRadiusDoubleValueConverter}}" |
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.
qq: this doesn't matter any longer because we are always running on a contract 7+ machine?
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.
Answered for myself! 19H1 is UniversalApiContract 8, so our checks were never necessary!
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.
Thanks for doing this!
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Fixed various small issues: * Made TabView bottom border span the entire window width * Made ScrollBar inner thumb rounded again * Made SplitButton look more like the new add tab button * Adjusted rename box height (24px, like the official compact sizing height) * Adjusted caption button colors to match Windows 11 * ColorPicker can now escape window bounds * Tweaked ColorPicker buttons (cherry picked from commit fa25dfb) Service-Card-Id: 81919698 Service-Version: 1.13
🎉 Handy links: |
🎉 Handy links: |
Fixed various small issues: