Skip to content

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

Merged
merged 5 commits into from
Oct 17, 2019

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Oct 12, 2019

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

After:
img

PR Context

PR Checklist

@SteveL-MSFT
Copy link
Member Author

@PoshChan please retry static

@PoshChan
Copy link
Collaborator

@SteveL-MSFT, successfully started retry of PowerShell-CI-static-analysis

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a 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

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Oct 12, 2019

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.

@iSazonov
Copy link
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? :-)

@iSazonov
Copy link
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.

@SteveL-MSFT
Copy link
Member Author

@PoshChan please retry static

@PoshChan
Copy link
Collaborator

@SteveL-MSFT, successfully started retry of PowerShell-CI-static-analysis

@SteveL-MSFT
Copy link
Member Author

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

@iSazonov
Copy link
Collaborator

@SteveL-MSFT I pulled #10778 to fix static CI.

@iSazonov
Copy link
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 :-)

@vexx32
Copy link
Collaborator

vexx32 commented Oct 12, 2019

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?

@SteveL-MSFT
Copy link
Member Author

@vexx32 we should probably have a new issue to discuss that

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 12, 2019

#10780 new issue for coloring.

@SteveL-MSFT SteveL-MSFT changed the title Update upgrade notification message to use color Update upgrade notification message Oct 15, 2019
@daxian-dbw
Copy link
Member

@joeyaiello Please review the user experience of the notification message.

@daxian-dbw
Copy link
Member

@bergmeister Can you please also review this?

Copy link
Contributor

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

Copy link
Contributor

@bergmeister bergmeister left a 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

@daxian-dbw daxian-dbw merged commit cb66974 into PowerShell:master Oct 17, 2019
@anmenaga 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
kilasuit pushed a commit to kilasuit/PowerShell that referenced this pull request Nov 9, 2019
@SteveL-MSFT SteveL-MSFT deleted the upgrade-message branch June 6, 2020 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants