-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
modify diff coloring for lower cognitive load #8572
Conversation
- snapshot "removed" lines are now red - received "added" lines are now yellow (neither good nor bad, just different)
Thanks @probablyup! Here's some thoughts: On colors: Your original suggestion was using blue and yellow for expected and received - I think that would be less controversial because if one of the colors is still green or red, each camp would want to claim it for either expected or received. I'm also wondering whether the colors are confusing people on non-snapshot diffs as well. Otherwise we might want to change it only for snapshots. Or maybe try it on snapshots, see how the reactions are, and then possibly change it for all diffs. On the header: @pedrottimark may be currently redesigning those I believe. Definitely shouldn't do changes to that in this same PR either way. |
Actually I suggested white and yellow, but thought white was maybe not obvious enough after the fact about the lines that were actually removed. |
Sorry I recalled that wrongly, should have looked at the threads again. |
Yeah, what Tim said about the header cannot change in this pull request because:
|
Has anyone in this discussion:
Changed Files in GitHub has a light background tint that is much easier on my eyes than the standard foreground colors in default macOS Terminal. |
@probablyup Thank you for bringing this back to the front burner. We agree that cognitive load is a problem to solve. Nielsen Norman Group writes:
With respect, I will suggest alternative colors as a starting point for discussion. For example, because limitations in my eyesight prevent dark theme, yellow on white would not work for me. Here is a picture of prototype line diff Here is a picture of prototype string diff added in #8569
The reason for magenta is similar to red but:
The reason for background color is contrast with default mode (see next comment) and look similar to Changed Files on GitHub to strengthen the analogy in testers understanding between snapshot review in terminal and code review in pull request. To allow other dependents of
|
For discussion is the concept that Jest needs to provide both color themes.
If that makes sense, then an important Developer Experience issue is how to make it easy for people to tell Jest which situation applies to this test run.
|
TBH I can't imagine many people using a CLI option or even a watch mode keybind - it has nowhere near the impact of e.g. pressing |
TLDR
Example pictures in following comments have baseline at left and improved at right Follow up on #8572 (comment) by @jeysal /cc @SimenB @thymikee
I agree with Tim that two modes is impractical DX and also insufficient in 80% of cases when people make incremental changes: each particular change to a snapshot could be progress or regress (and the worst case for mistaken decisions seems like snapshot with some of both) And thoughts from team chat by @cpojer
We also invite @aaronabramov @gaearon to review prototype according to #2347 (comment)
Let’s adjust that principle to display report in console when snapshot test fails with appearance:
|
Since #8569 a report highlights substring differences for snapshot if received is string and snapshot seems like serialized string or from custom serializer: Otherwise report displays line diff: |
Here a preview how review colors might look in future data-driven diff The report displays changes more clearly for deep equality matchers: Similar improvement for updatable criterion seems possible if inline “whatever” snapshot can consist of literal array or object notation instead of serialization: |
By the way:
const snapshotColor = has256 ? chalk.magenta.bgAnsi256(225) : chalk.magenta;
const receivedColor = has256 ? chalk.green.bgAnsi256(194) : chalk.green; |
@pedrottimark that definitely is easier to read than trying to hunt and peck in the full line diff to figure out what changed. I'm not really sure about the magenta/green though... I wish we had strikethrough support in more terminals, that would be a much more obvious solution. |
@probablyup Your constructive critique is welcome, because I’m not sure either :) Did you have any thoughts which if any of the alternative labels communicate effectively? |
@pedrottimark what about something like this? (see the red notes for what I wasn't able to change in the screenshot) Basically the existing value could be green (maybe even de-emphasize it more by not even giving it a background) and then the incoming change would be the magenta. When it's flipped so the received value is green, that's confusing to the reader because it makes them think the original snapshot is what's incorrect. |
Here is another variation:
As before, goals include:
The background received color in this version has hue 150° to give it a hint of green Although teal has xterm number That requires some more follow-up research to see if it achieves goals:
|
@probablyup Thank you for having brought this problem back to the front burner! |
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. |
addresses #5430