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

Make tab header a custom control #8227

Merged
10 commits merged into from
Nov 20, 2020
Merged

Make tab header a custom control #8227

10 commits merged into from
Nov 20, 2020

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Nov 11, 2020

This PR makes the Header of TabViewItem a custom user control.

Validation Steps Performed

Manual testing

Closes #8201

@PankajBhojwani PankajBhojwani changed the title [DRAFT] Make tab header a custom control Make tab header a custom control Nov 11, 2020
@PankajBhojwani PankajBhojwani marked this pull request as ready for review November 11, 2020 22:15
@ghost ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Nov 11, 2020
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 11, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 13, 2020
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.

Overall this looks pretty good, but I wanna block for discussion on passing the TabBase to the HeaderControl, so hopefully more of this can just be magic

src/cascadia/TerminalApp/TabHeaderControl.cpp Outdated Show resolved Hide resolved
@@ -37,6 +37,17 @@ namespace winrt::TerminalApp::implementation

_MakeTabViewItem();
_CreateContextMenu();

// Add an event handler for the header control to tell us when they want their title to change
_headerControl.HeaderTitleChanged([weakThis = get_weak()](auto&& title) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to author a custom event (HeaderTitleChanged) for this? Since Title is observable, shouldn't we be able to do this with the automatic PropertyChanged event?

For ex, TerminalPage.cppL1143-1163, we'd probably do something similar:

_headerControl.PropertyChanged([weakThis=get_weak()](auto&&, const WUX::Data::PropertyChangedEventArgs& args) {
    if (auto tab{ weakThis.get() })
    {
        if (args.PropertyName() == L"Title")
        {
            tab->SetTabText(title);
        }
    }
});

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani Nov 16, 2020

Choose a reason for hiding this comment

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

Ah I should rename this event - the purpose of this event is to let the hosting tab know that the user tried to change the title to x, and once the tab receives that event it tells the headerControl to change its title to x.

The reason for this is so its always the tab that has ownership over the title

(the event was named this way in the first iteration when the headerControl indeed just changed the title and simply wanted to let the tab know about it, now its more like it tells the tab/asks the tab's permission to change its title)

src/cascadia/TerminalApp/TabHeaderControl.idl Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 16, 2020
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.

Yea, these are all nits. Thanks for finally biting the bullet on this 🙌

src/cascadia/TerminalApp/TabHeaderControl.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.xaml Outdated Show resolved Hide resolved
Comment on lines 15 to 28
<FontIcon x:Name="HeaderZoomIcon"
FontFamily="Segoe MDL2 Assets"
Visibility="Collapsed"
Glyph="&#xE8A3;"
FontSize="12"
Margin="0,0,8,0"/>
<TextBlock x:Name="HeaderTextBlock"
Visibility="Visible"
Text="{x:Bind Title, Mode=OneWay}"/>
<TextBox x:Name="HeaderRenamerTextBox"
Visibility="Collapsed"
MinHeight="0"
Padding="4,0,4,0"
Margin="0,-8,0,-8"
Copy link
Member

Choose a reason for hiding this comment

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

let's make sure this still works under accessibility. @carlos-zamora wanna help out?

src/cascadia/TerminalApp/TabHeaderControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabHeaderControl.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 20, 2020
@DHowett
Copy link
Member

DHowett commented Nov 20, 2020

Sorry, I hate to block for things i mostly called nits 😄

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 20, 2020
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.

Love it! Thanks 😄

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 20, 2020
@ghost
Copy link

ghost commented Nov 20, 2020

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 a77b494 into main Nov 20, 2020
@ghost ghost deleted the dev/pabhoj/tab_header_control branch November 20, 2020 17:16
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Nov 22, 2020
This PR makes the Header of TabViewItem a custom user control.

## Validation Steps Performed
Manual testing

Closes microsoft#8201
@zadjii-msft zadjii-msft mentioned this pull request Nov 30, 2020
6 tasks
@ghost
Copy link

ghost commented Jan 28, 2021

🎉Windows Terminal Preview v1.6.10272.0 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-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the TabViewItemHeader a custom control
3 participants