-
Notifications
You must be signed in to change notification settings - Fork 383
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
Finalise new -ReportSummary switch #895
Finalise new -ReportSummary switch #895
Conversation
…f error, warning, and info results
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.
I think the summary is fine and has some utility. It does not need to be used, but some may find it very useful.
However, one of the things we need to keep in mind when color is used explicitly is that we keep accessibility in mind as the colors we choose may not be accessible for all.
Further, some window color themes may be set in such a way that the chosen colors may not be visible at all. For example, a red background would have difficulties with red foreground text.
I think that the colors used by Write-Host (and host.writeline
) need to be those found in $Host.PrivateData
rather than hardcoded. We may not be able to provide the ultimate in configuration, but we can use the scheme in use. This will provide the best accessibility for our customers. It is also consistent with the way PowerShell uses colors.
Sounds reasonable. I was actually thinking about the colour problem but saw it at first rather as an edge case. I will take a look at your pointer with |
@JamesWTruher I have applied your suggestions and the PR is up for re-rereview |
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.
this looks fine, please go ahead and merge!
@JamesWTruher Thanks but GitHub says that I do not have write access to protected branches like |
I'll try to fix the merge permissions - I want to be sure that you can do that. |
Thanks. But we should still continue the process of getting approval from each other in PRs before merging. |
Nevermind, I think I found it and will check when I get into the office. |
PR Summary
Fixes #857 and closes #868
This is the tweak of the new
ReportSummary
of PR #868 by @StingyJack that:-ReportSummary
switch and adapting the library test function mock for consistencyPR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
x
between the square brackets. Please mark anything not applicable to this PRNA
.Visual Studio 2017
image firstWIP:
to the beginning of the title and remove the prefix when the PR is ready