FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Update upgrade notification message by SteveL-MSFT · Pull Request #10777 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Update upgrade notification message - #10777

Merged
Dongbo Wang (daxian-dbw) merged 5 commits into
PowerShell:masterfrom
SteveL-MSFT:upgrade-message
Oct 17, 2019
ConversationCommits (5)ChecksFiles changed
Merged

Update upgrade notification message#10777
Dongbo Wang (daxian-dbw) merged 5 commits into
PowerShell:masterfrom
SteveL-MSFT:upgrade-message

Conversation

Steve Lee (SteveL-MSFT) commented Oct 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 word stable in place of preview.

Before:

After:

PR Context

PR Checklist

Steve Lee (SteveL-MSFT) commented Oct 12, 2019

Copy link
Copy Markdown
Member Author

PoshChan-Bot (@PoshChan) please retry static

PoshChan-Bot (PoshChan) commented Oct 12, 2019

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT), successfully started retry of PowerShell-CI-static-analysis

TylerLeonhardt reviewed Oct 12, 2019
View reviewed changes

TylerLeonhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Due to feedback from another PR I strongly encourage not going with the hard-coded yellow

Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Outdated

TylerLeonhardt commented Oct 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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.

Ilya (iSazonov) commented Oct 12, 2019

Copy link
Copy Markdown
Collaborator

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.
What will be next? Advertising banners? :-)

Ilya (iSazonov) commented Oct 12, 2019

Copy link
Copy Markdown
Collaborator

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.
Unix utilities have --color key but not pwsh.
I don’t like that we began to saturate the codewith color without a common approach.

Steve Lee (SteveL-MSFT) commented Oct 12, 2019

Copy link
Copy Markdown
Member Author

PoshChan-Bot (@PoshChan) please retry static

PoshChan-Bot (PoshChan) commented Oct 12, 2019

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT), successfully started retry of PowerShell-CI-static-analysis

Steve Lee (SteveL-MSFT) commented Oct 12, 2019

Copy link
Copy Markdown
Member Author

Ilya (@iSazonov) I've started adding more color settings to $host.PrivateData (it's unfortunate that member is named that...) so users can opt out or customize.

Ilya (iSazonov) commented Oct 12, 2019

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) I pulled #10778 to fix static CI.

Ilya (iSazonov) commented Oct 12, 2019

Copy link
Copy Markdown
Collaborator

I've started adding more color settings to $host.PrivateData (it's unfortunate that member is named that...) so users can opt out or customize.

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

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Agreed. We shouldn't bury it in a $host API unless we also plan on exposing it via a cmdlet when it is reasonably feature-complete. 🙂

Couldn't we store color settings in the settings JSON file?

Steve Lee (SteveL-MSFT) commented Oct 12, 2019

Copy link
Copy Markdown
Member Author

Rain Sallow (/u/ta11ow) (@vexx32) we should probably have a new issue to discuss that

Ilya (iSazonov) commented Oct 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

#10780 new issue for coloring.

Steve Lee (SteveL-MSFT) changed the title Update upgrade notification message to use color Update upgrade notification message Oct 15, 2019

Dongbo Wang (daxian-dbw) commented Oct 15, 2019

Copy link
Copy Markdown
Member

Joey Aiello (@joeyaiello) Please review the user experience of the notification message.

Dongbo Wang (daxian-dbw) commented Oct 15, 2019

Copy link
Copy Markdown
Member

Christoph Bergmeister (@bergmeister) Can you please also review this?

Joey Aiello (joeyaiello) approved these changes Oct 16, 2019
View reviewed changes

Joey Aiello (joeyaiello) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

String looks fine to me for now, we can easily iterate it over time. Steve Lee (@SteveL-MSFT) and I discussed possibly doing an "upgrade doc" that we can point to generically (/cc Sean Wheeler (@sdwheeler)) that would describe how updating should be done on each platform.

Christoph Bergmeister (bergmeister) approved these changes Oct 17, 2019
View reviewed changes

Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Lgtm, nice colour block

Dongbo Wang (daxian-dbw) merged commit cb66974 into PowerShell:master Oct 17, 2019
Andrew (anmenaga) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 22, 2019
Ryan Yates (kilasuit) pushed a commit to kilasuit/PowerShell that referenced this pull request Nov 9, 2019
Update upgrade notification message (PowerShell#10777)
3c59b4e
Steve Lee (SteveL-MSFT) deleted the upgrade-message branch June 6, 2020 02:31
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Update upgrade notification message (PowerShell#10777)
641c3bb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Ilya (iSazonov) iSazonov left review comments
Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
Andrew (anmenaga) Awaiting requested review from anmenaga
+5 more reviewers
TylerLeonhardt TylerLeonhardt left review comments
Rain Sallow (/u/ta11ow) (vexx32) vexx32 left review comments
PoshChan-Bot (PoshChan) PoshChan left review comments
Joey Aiello (joeyaiello) joeyaiello approved these changes
Christoph Bergmeister (bergmeister) bergmeister approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

Dongbo Wang (daxian-dbw)

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.0.0-preview.5

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL