fix(cli): colored text is unreadable when using light themes #5250

netroy · 2019-11-28T22:49:40Z

For someone using a light-colored background on their terminal, white colored text is unreadable.
This change respects the user's foreground color preferences.

Screenshots

Light color scheme

Dark color scheme

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mergify · 2019-11-28T22:50:17Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-28T23:13:00Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shivlaks · 2019-11-29T02:49:28Z

@netroy - mentioned this in #5242 but we have the --no-color option to support disabling color and style from console output.

Is there a gap that the option does not cover?

shivlaks

unsure if this change is still needed as we support disabling color and style from console output

netroy · 2019-11-29T07:45:54Z

@shivlaks This change is still relevant for users with light colour scheme, who do want to use colours.
I suppose it comes down to the question: How many such users of CDK exist? and, If CDK should provide them with the same options as everyone else? or, Is it safe to assume that users of CDK predominately prefer dark color schemes.

shivlaks · 2019-11-29T08:38:07Z

@shivlaks This change is still relevant for users with light colour scheme, who do want to use colours.
I suppose it comes down to the question: How many such users of CDK exist? and, If CDK should provide them with the same options as everyone else? or, Is it safe to assume that users of CDK predominately prefer dark color schemes.

@netroy that makes more sense to me. Users who want colors in light-colored terminals should have a better experience. Can you add some screenshots of what this looks like (with light and dark colored terminals).

netroy · 2019-11-29T09:16:46Z

I just realized that logging.print is using console.error, which does it's own set of colors (in my case red).
I do believe we need a fix. But, I doubt the changes in this PR are alone going to fix it.

Also, the colors.white makes sense now.
I think that it might have been done to override the color red from console.error.

@shivlaks if you believe that we should fix this, I can update logging.ts to directly use stdout/stderr instead, since none of the console API features are used, & the current code seems to be working around the defaults of that API. I can add some tests as well.

Pull request has been modified.

aws-cdk-automation · 2019-11-29T16:06:38Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-29T16:51:46Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-12-16T12:33:23Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:00:49Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:07Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:49Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:23Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:28Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-12-17T12:30:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shivlaks

Can you update the body of the request to capture the rationale as well as the screenshots with this implementation for posterity
There's a merge conflict to straighten out
I'm wondering if there's something we could be doing to test this?

aws-cdk-automation · 2019-12-18T04:13:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

netroy · 2019-12-19T16:57:28Z

* Can you update the body of the request to capture the rationale as well as the screenshots with this implementation for posterity

done

* There's a merge conflict to straighten out

done

* I'm wondering if there's something we could be doing to test this?

I'm open to ideas. The only thing i see, that I can test, is the behavior of the debug function. But testing that behavior seems unrelated to the changes in this PR.

Pull request has been modified.

aws-cdk-automation · 2020-01-07T17:23:53Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-08T21:07:48Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

netroy · 2020-01-10T14:16:15Z

@shivlaks just rebased this again.
Can you please have a look at this again?
Thanks

aws-cdk-automation · 2020-01-10T14:51:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-14T18:07:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-16T11:20:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/aws-cdk/lib/logging.ts

respect the user's color scheme

console methods use their own set of colors, which CDK then has to override. Switching to stdout/stderr streams gets rid of this step.

Pull request has been modified.

aws-cdk-automation · 2020-01-27T10:26:00Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2020-01-28T04:12:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

netroy · 2020-01-28T08:48:52Z

Thanks a lot @shivlaks 🤗

netroy requested a review from RomainMuller as a code owner November 28, 2019 22:49

shivlaks previously requested changes Nov 29, 2019

View reviewed changes

netroy force-pushed the fix-5242 branch from 040d02c to efcc045 Compare November 29, 2019 15:43

netroy changed the title ~~fix(cli): don't force the color white as the foreground color~~ fix(cli): remove console.* from logging, & fix color issues for light themes Nov 29, 2019

netroy requested a review from shivlaks November 29, 2019 15:50

netroy force-pushed the fix-5242 branch from efcc045 to ca52eba Compare November 29, 2019 16:22

SomayaB assigned shivlaks Nov 29, 2019

netroy force-pushed the fix-5242 branch from ca52eba to fc93fae Compare December 17, 2019 12:05

shivlaks previously requested changes Dec 18, 2019

View reviewed changes

netroy force-pushed the fix-5242 branch from fc93fae to f40c5ff Compare December 18, 2019 03:39

netroy force-pushed the fix-5242 branch from f40c5ff to 6a92dd1 Compare December 23, 2019 11:59

netroy force-pushed the fix-5242 branch from 96b08f9 to 265d2e8 Compare January 7, 2020 16:48

netroy force-pushed the fix-5242 branch from 265d2e8 to 58476f6 Compare January 8, 2020 20:32

netroy force-pushed the fix-5242 branch from 58476f6 to e4fe609 Compare January 10, 2020 14:15

netroy force-pushed the fix-5242 branch from e4fe609 to 02fcb88 Compare January 14, 2020 17:22

netroy requested a review from shivlaks January 14, 2020 17:22

netroy force-pushed the fix-5242 branch from 02fcb88 to 8550017 Compare January 16, 2020 10:44

shivlaks previously requested changes Jan 22, 2020

View reviewed changes

packages/aws-cdk/lib/logging.ts Show resolved Hide resolved

netroy requested a review from shivlaks January 27, 2020 09:46

netroy added 2 commits January 27, 2020 10:47

fix(cli): don't force the color white as the foreground color

99a0795

respect the user's color scheme

feat(cli): stop using console.* for logging

185b247

console methods use their own set of colors, which CDK then has to override. Switching to stdout/stderr streams gets rid of this step.

netroy force-pushed the fix-5242 branch from 8550017 to 185b247 Compare January 27, 2020 09:47

shivlaks previously approved these changes Jan 28, 2020

View reviewed changes

shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Jan 28, 2020

shivlaks changed the title ~~fix(cli): remove console.* from logging, & fix color issues for light themes~~ fix(cli): colored text is unreadable when using light themes Jan 28, 2020

Merge branch 'master' into fix-5242

9bcedd3

shivlaks approved these changes Jan 28, 2020

View reviewed changes

shivlaks merged commit b4573ef into aws:master Jan 28, 2020

netroy deleted the fix-5242 branch January 28, 2020 08:48

shivlaks mentioned this pull request May 26, 2020

Standard CDK log messages are written to the StandardError stream #7717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): colored text is unreadable when using light themes #5250

fix(cli): colored text is unreadable when using light themes #5250

netroy commented Nov 28, 2019 •

edited

Loading

mergify bot commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

shivlaks commented Nov 29, 2019

shivlaks left a comment

netroy commented Nov 29, 2019

shivlaks commented Nov 29, 2019 •

edited

Loading

netroy commented Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 17, 2019

shivlaks left a comment

aws-cdk-automation commented Dec 18, 2019

netroy commented Dec 19, 2019 •

edited

Loading

aws-cdk-automation commented Jan 7, 2020

aws-cdk-automation commented Jan 8, 2020

netroy commented Jan 10, 2020

aws-cdk-automation commented Jan 10, 2020

aws-cdk-automation commented Jan 14, 2020

aws-cdk-automation commented Jan 16, 2020

aws-cdk-automation commented Jan 27, 2020

aws-cdk-automation commented Jan 28, 2020

netroy commented Jan 28, 2020

fix(cli): colored text is unreadable when using light themes #5250

fix(cli): colored text is unreadable when using light themes #5250

Conversation

netroy commented Nov 28, 2019 • edited Loading

Screenshots

Light color scheme

Dark color scheme

mergify bot commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

AWS CodeBuild CI Report

shivlaks commented Nov 29, 2019

shivlaks left a comment

Choose a reason for hiding this comment

netroy commented Nov 29, 2019

shivlaks commented Nov 29, 2019 • edited Loading

netroy commented Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 17, 2019

AWS CodeBuild CI Report

shivlaks left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 18, 2019

AWS CodeBuild CI Report

netroy commented Dec 19, 2019 • edited Loading

aws-cdk-automation commented Jan 7, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 8, 2020

AWS CodeBuild CI Report

netroy commented Jan 10, 2020

aws-cdk-automation commented Jan 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 14, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 16, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 28, 2020

AWS CodeBuild CI Report

netroy commented Jan 28, 2020

netroy commented Nov 28, 2019 •

edited

Loading

shivlaks commented Nov 29, 2019 •

edited

Loading

netroy commented Dec 19, 2019 •

edited

Loading