-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Update upgrade notification message #10777
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
Conversation
@PoshChan please retry static |
@SteveL-MSFT, successfully started retry of |
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.
Due to feedback from another PR I strongly encourage not going with the hard-coded yellow
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
Also, this would be the second location the inverse escape sequence is hard coded in our code... We need to figure out what we're going to do about that in the future. |
Coloring is very sensitive area. Windows has a special team for this. I'd prefer classic color scheme. This color kaleidoscope is distracting and annoying. |
Coloring is nice for demo on a conference stand. For normal daily work, when the console is started dozens of times, it can tire your eyes. |
@PoshChan please retry static |
@SteveL-MSFT, successfully started retry of |
@iSazonov I've started adding more color settings to |
@SteveL-MSFT I pulled #10778 to fix static CI. |
Oh, it would be great to have modern design (RFC?) - PowerShell is power and I'd expect that we could do coloring gracefully, smart and powershelly :-) |
Agreed. We shouldn't bury it in a Couldn't we store color settings in the settings JSON file? |
@vexx32 we should probably have a new issue to discuss that |
#10780 new issue for coloring. |
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
@joeyaiello Please review the user experience of the notification message. |
@bergmeister Can you please also review this? |
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/resources/ManagedEntranceStrings.resx
Show resolved
Hide resolved
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.
String looks fine to me for now, we can easily iterate it over time. @SteveL-MSFT and I discussed possibly doing an "upgrade doc" that we can point to generically (/cc @sdwheeler) that would describe how updating should be done on each platform.
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.
Lgtm, nice colour block
PR Summary
If terminal doesn't support VT, no colors are used. Also added aka.ms shortcut. Padding had to be calculated to make sure it looks nice which means
if the message line lengths change, the code may need to be updated as it expects the first line to be longest and the first and last line to include
the release tag. The
stable
version is the same but the wordstable
in place ofpreview
.Before:

After:

PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.