Skip to content
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

Simplify highlight for leading and trailing spaces in jest-diff #4553

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

  • Replace explicit background colors with inverse for removed, added, in-changed lines.
  • Mid-course change: add bgYellow for unchanged lines which didn’t have highlight. The dim stack trace and gray lines caused a perceptual conflict instead of a contrast. Also as Scott said, gray was not light enough. I think yellow looks better on Windows. It looks like pollen on the following illustrations from MacOS Terminal. Also better not to churn snapshots of diffs.

Removed lines are bgGreen at left and inverse of green at right, that is the same:
deleted

Added lines are bgRed at left and inverse of red at right, that is the same:
inserted

Indentation-changed lines are bgCyan at left and inverse of cyan at right, that is the same:
inchanged

Unchanged lines are unhighlighted at left and bgYellow at right:
unchanged

Thank y’all for reviewing incremental improvements even when urgent problems need solutions.

Next: similar change in jest-matcher-utils but a few things puzzled me in expect

Test plan

Updated 4 snapshot in 1 test suite (renamed one of them)

P.S. Give my thanks to the Yarn team for improved links so previous PR passed AppVeyor CI

@codecov-io
Copy link

Codecov Report

Merging #4553 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4553      +/-   ##
==========================================
- Coverage   56.04%   56.01%   -0.03%     
==========================================
  Files         185      185              
  Lines        6268     6264       -4     
  Branches        3        3              
==========================================
- Hits         3513     3509       -4     
  Misses       2754     2754              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-diff/src/diff_strings.js 98.83% <100%> (-0.06%) ⬇️
packages/pretty-format/src/plugins/convert_ansi.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23c1e10...be0e2fe. Read the comment docs.

@cpojer cpojer merged commit 9548c16 into jestjs:master Sep 28, 2017
@cpojer
Copy link
Member

cpojer commented Sep 28, 2017

super awesome.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants