Skip to content
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

Merged
5 commits merged into from Apr 21, 2022
Merged

Misc visual fixes #12916

5 commits merged into from Apr 21, 2022

Conversation

ghost
Copy link

@ghost ghost commented Apr 16, 2022

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

@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 18, 2022

work in progress comparison shots:

Element Before After
Made TabView bottom border span the entire window width

image
image

image
image

Made ScrollBar inner thumb rounded again

image

image

Made SplitButton look more like the new add tab button

gh-12916-newtab-before

gh-12916-newtab-after

Adjusted rename box height (24px, like the official compact sizing height)

image

image

Adjusted caption button colors to match Windows 11 before after
ColorPicker can now escape window bounds

image

image

Tweaked ColorPicker buttons

image

image

  • Tweaked ColorPicker buttons
    • Maybe there was a slight hover effect added? Check the code.
    • ‼️ There's definitely a crash selecting a color button for a tab. WinDBG is still hosed so I dunno where that crash is exactly.
  • I need to find a way to compare the new caption hover colors with other apps. Not sure these are right.

@zadjii-msft
Copy link
Member

  • Edgium:
    image
  • notepad rejuv:
    image
  • Terminal before:
    image
  • Terminal after:
    image
  • XCG windowing sample:
    image

Okay so it totally looks like they changed the caption button colors on us. Cool.

@zadjii-msft zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Needs-Discussion Something that requires a team discussion before we can proceed labels Apr 18, 2022
Copy link
Member

@zadjii-msft zadjii-msft left a 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 -->
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

@zadjii-msft
Copy link
Member

 	TerminalApp.dll!winrt::impl::consume_Windows_UI_Xaml_Shapes_IShape<winrt::Windows::UI::Xaml::Shapes::Rectangle>::Fill() Line 201	C++
>	TerminalApp.dll!winrt::TerminalApp::implementation::ColorPickupFlyout::ColorButton_Click(const winrt::Windows::Foundation::IInspectable & sender, const winrt::Windows::UI::Xaml::RoutedEventArgs & __formal) Line 38	C++
 	[External Code]	
 	TerminalApp.dll!winrt::impl::delegate<winrt::Windows::UI::Xaml::RoutedEventHandler,`winrt::TerminalApp::implementation::ColorPickupFlyoutT<winrt::TerminalApp::implementation::ColorPickupFlyout>::Connect'::`15'::<lambda_10>>::Invoke(void * sender, void * e) Line 4672	C++

    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

@ghost
Copy link
Author

ghost commented Apr 18, 2022

Color buttons now have a colored background and an overlay with the default button style, so that they change on hover/press.
Because they use the default colors, they'll look a bit different on light mode vs dark mode.
I think I should also update the buttons in Settings to work like this.

@ghost
Copy link
Author

ghost commented Apr 18, 2022

I think what makes the add tab button look squished is that it's really wide, and I'm not sure if I can customize the width without retemplating.
image

@cinnamon-msft
Copy link
Contributor

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.

@ghost
Copy link
Author

ghost commented Apr 19, 2022

Turns out there is actually a way to resize the buttons:
image
image
This matches the TabView button. Is this ok? Otherwise I'll revert the design.

@zadjii-msft
Copy link
Member

This matches the TabView button. Is this ok?

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.

Copy link

@PrelevatedInsider17763 PrelevatedInsider17763 left a 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.

Copy link

@PrelevatedInsider17763 PrelevatedInsider17763 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

access

@Shomnipotence

This comment was marked as off-topic.

@zadjii-msft
Copy link
Member

@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.

Copy link
Member

@zadjii-msft zadjii-msft left a 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.

@zadjii-msft zadjii-msft added Needs-Second It's a PR that needs another sign-off and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Apr 21, 2022
@@ -351,11 +352,9 @@
<ControlTemplate x:Key="VerticalThumbTemplate"
TargetType="Thumb">
<Rectangle x:Name="ThumbVisual"
contract7NotPresent:RadiusX="{Binding Source={ThemeResource ScrollBarCornerRadius}, Converter={StaticResource TopLeftCornerRadiusDoubleValueConverter}}"
Copy link
Member

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?

Copy link
Member

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!

Copy link
Member

@DHowett DHowett left a 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!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 21, 2022
@ghost
Copy link

ghost commented Apr 21, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit fa25dfb into microsoft:main Apr 21, 2022
carlos-zamora pushed a commit that referenced this pull request May 17, 2022
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
@ghost
Copy link

ghost commented May 24, 2022

🎉Windows Terminal v1.13.1143 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented May 24, 2022

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants