Skip to content

Teach info bars to be dismissed permanently#11139

Merged
4 commits merged intomicrosoft:mainfrom
Don-Vito:10798-introduce-dismissable-messages
Sep 10, 2021
Merged

Teach info bars to be dismissed permanently#11139
4 commits merged intomicrosoft:mainfrom
Don-Vito:10798-introduce-dismissable-messages

Conversation

@Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Sep 3, 2021 •
edited by zadjii-msft
Loading

Summary of the Pull Request

  • Introduces info bar shown upon session failure,
    that guides the user how to configure termination behavior
    • Allows this info bar to be dismissed permanently (choice stored in state)
  • Allows "keyboard service" info bar to be dismissed permanently

PR Checklist

Detailed Description of the Pull Request / Additional comments

UI:

  • Introduce an additional info bar for "close on exit" configuration tip
    • Stack this bar after "keyboard service" bar
  • Add "Don't show again" button to both bars

Dismiss Permanently:

  • Introduce a set of "dismissed messages" to the Application State
  • Add verification the message is not dismissed before showing an info bar
  • "Don't show again" persists the choice under "dismissed messages"

Wiring the Info Bar:

  • Register TerminalPage on TermControl's ConnectionStateChanged event
  • Once event is triggered check whether the state is failure
  • If so and the message was not dismissed permanently, show the info bar

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Sep 3, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Sep 3, 2021

image

carlos-zamora
carlos-zamora requested changes Sep 7, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Just a few small things before I approve

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 7, 2021
@zadjii-msft
Copy link
Member

zadjii-msft commented Sep 7, 2021

Okay I need top put aside more time for a full review, but this is legit.

<crazy idea hat>

How hard would it be to add a link to the docs (like, to https://docs.microsoft.com/en-us/windows/terminal/customize-settings/profile-advanced#profile-termination-behavior) to that infobar? We could generate the fwlink if needed

zadjii-msft
zadjii-msft approved these changes Sep 8, 2021
View reviewed changes
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.

Besides wording nits, I'm cool with the rest of the code here. I'll leave it to the rest of the team to sort out the specific wording (/cc @cinnamon-msft)

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 9, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Sep 9, 2021 •
edited
Loading

How hard would it be to add a link to the docs (like, to https://docs.microsoft.com/en-us/windows/terminal/customize-settings/profile-advanced#profile-termination-behavior) to that infobar? We could generate the fwlink if needed

@zadjii-msft - this is not very hard. We can do something like

image

but we need to decide if we want to follow up by opening the settings 😊.... or... we can do all of them:
image

In any case suggest to do it in a follow up. I can open a ticket for it.. and proceed with it if the team is interested.

@zadjii-msft zadjii-msft mentioned this pull request Sep 9, 2021
Open
@cinnamon-msft
Copy link
Contributor

cinnamon-msft commented Sep 10, 2021

I am a fan of this. :)

carlos-zamora
carlos-zamora approved these changes Sep 10, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Perfect. Let's do this

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 10, 2021
@ghost
Copy link

ghost commented Sep 10, 2021

Hello @carlos-zamora!

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 a900aba into microsoft:main Sep 10, 2021
@zadjii-msft zadjii-msft mentioned this pull request Sep 13, 2021
Closed
DHowett pushed a commit that referenced this pull request Oct 13, 2021
@Don-Vito @DHowett
Teach info bars to be dismissed permanently (#11139)
603122f 
* Allows "keyboard service" info bar to be dismissed permanently

UI:
* Add "Don't show again" button to the keyboard info bar

Dismiss Permanently:
* Introduce a set of "dismissed messages" to the Application State
* Add verification the message is not dismissed before showing an info bar
* "Don't show  again" persists the choice under "dismissed messages"

(cherry-picked from commit a900aba)
@ghost
Copy link

ghost commented Oct 20, 2021

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

Handy links:

@ghost ghost mentioned this pull request Oct 20, 2021
Closed
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal Preview v1.12.2922.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

Reviewers

@DHowett DHowett DHowett left review comments

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

+1 more reviewer

@cinnamon-msft cinnamon-msft cinnamon-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make exit code behavior more disacoverable Allow users to suppress ctfmon service notification (perhaps store in app state?)

5 participants

@Don-Vito @zadjii-msft @cinnamon-msft @DHowett @carlos-zamora