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

modify diff coloring for lower cognitive load #8572

Closed
wants to merge 1 commit into from

Conversation

quantizor
Copy link
Contributor

@quantizor quantizor commented Jun 15, 2019

  • snapshot "removed" lines are now red
  • received "added" lines are now yellow (neither good nor bad, just different)

addresses #5430

- snapshot "removed" lines are now red
- received "added" lines are now yellow (neither good nor bad, just different)
@quantizor
Copy link
Contributor Author

quantizor commented Jun 15, 2019

I'm kind of tempted to remove the header as well:

Screen Shot 2019-06-15 at 5 05 20 PM

The top of the error very clearly says snapshot mismatch so there's enough context to know the removed and added lines are referring to the snapshot in general.

@jeysal
Copy link
Contributor

jeysal commented Jun 16, 2019

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.

@quantizor
Copy link
Contributor Author

Your original suggestion was using blue and yellow

Actually I suggested white and yellow, but thought white was maybe not obvious enough after the fact about the lines that were actually removed.

@jeysal
Copy link
Contributor

jeysal commented Jun 16, 2019

Actually I suggested white and yellow,

Sorry I recalled that wrongly, should have looked at the threads again.
I think red and green are too controversial and white is too close to the (dim) unaffected lines, so we'll need some other color scheme.
Personally I'm in the "correct as it is" camp, but happy with changing it to something "neutral".

@pedrottimark
Copy link
Contributor

Yeah, what Tim said about the header cannot change in this pull request because:

  • Until either jest-diff returns explicit result that arguments are not diffable, or we finish guarding to prevent calling it in those cases, some code depends on presence or absence of header.
  • If future version changes default colors or provides options, then the header seems even more important than before.
  • For future data-driven diff (and maybe even line diff) the header might summarize results:

countDiff-change

@pedrottimark
Copy link
Contributor

Has anyone in this discussion:

  • used 256 and Truecolor color support in chalk package?
  • know how widely it is supported in terminal apps for our primary audience?

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.

@pedrottimark
Copy link
Contributor

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

Build on existing mental models: People already have mental models about how websites work, based on their past experiences visiting other sites. When you use labels and layouts that they've encountered on other websites, you reduce the amount of learning they need to do on your site.

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

8572 line 31

Here is a picture of prototype string diff added in #8569

8572 string 31

  • Snapshot: chalk.magenta.bgAnsi256(219)
  • Received: chalk.green.bgAnsi256(194)

The reason for magenta is similar to red but:

  • less directive that snapshot is incorrect (worst case is when received is only partly correct)
  • possibly less difficult for people who cannot distinguish red from green (research pending)
  • it looks nice :)

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 jest-diff flexibility to decide differently:

@pedrottimark
Copy link
Contributor

pedrottimark commented Jun 21, 2019

For discussion is the concept that Jest needs to provide both color themes.

Snapshot is green consistent with other matchers if it is more likely to be the correct criterion:

  • When you run tests before you make changes, any failed test is unexpected
  • When you run tests while you refactor code, most failed tests will pass again at the end

Received is green if it becomes the correct criterion upon update because changes to code affect behavior. The principle of cognitive load implies the report that Jest displays in terminal needs to be similar to other tools for code review.

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.

  • I suggest as a hypothesis that current behavior is default
  • What is a clear and concise CLI option and matching key for watch mode to easily switch modes?
  • Does that imply the key for watch mode is a toggle to switch modes, or are there two keys?

@jeysal
Copy link
Contributor

jeysal commented Jun 24, 2019

  • What is a clear and concise CLI option and matching key for watch mode to easily switch modes?
  • Does that imply the key for watch mode is a toggle to switch modes, or are there two keys?

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 a.
The fish design doc would tell us to figure out what the user wants, but I don't see how we could do that here (and even for a given snapshot mismatch situation there is hardly a consent on which way around the colors would be better), so I think compromise via different colors (at least trying it out) is a good course of action.

@pedrottimark
Copy link
Contributor

TLDR

  1. Prototype colors are neither opposite to other matchers nor identical to community tools:

    • Snapshot is magenta with light background
    • Received is green with light background
  2. Please critique 3 possible labels or suggest alternatives:

baseline 1 2 3
Snapshot Snapshot (changing from) Snapshot (change from) Snapshot: change from?
Received Received (updating to) Received (update to) Received: update to?

Example pictures in following comments have baseline at left and improved at right


Follow up on #8572 (comment) by @jeysal /cc @SimenB @thymikee

I can't imagine many people using a CLI option or even a watch mode keybind

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

I am not sure reversing the colors will have the desired effect but trying out different ones would be interesting

The point is though that you should turn on your brain and don't just pattern match and blindly run -u

We also invite @aaronabramov @gaearon to review prototype according to #2347 (comment)

assuming that what is stored on disk is always the source of truth

Let’s adjust that principle to display report in console when snapshot test fails with appearance:

  • similar to GitHub Changed Files (as name brand, and other community tools as generic brand :) because you use similar reasoning to evaluate changes before updating snapshot as review after opening pull request
  • different from other matchers to minimize confusion about meaning of green

@pedrottimark
Copy link
Contributor

Since #8569 a report highlights substring differences for snapshot if received is string and snapshot seems like serialized string or from custom serializer:

8572b string

Otherwise report displays line diff:

8572b line

@pedrottimark
Copy link
Contributor

Here a preview how review colors might look in future data-driven diff

The report displays changes more clearly for deep equality matchers:

8572b d3 toEqual

Similar improvement for updatable criterion seems possible if inline “whatever” snapshot can consist of literal array or object notation instead of serialization:

8572b d3 toMatchInlineWhatever

@pedrottimark
Copy link
Contributor

By the way:

  • Notice that matcher hint API in jest-matcher-utils will need optional color properties
  • magenta hue=300° is complement to green hue=120°
  • I found code for lighter magenta background color than in early first round of prototyping
const snapshotColor = has256 ? chalk.magenta.bgAnsi256(225) : chalk.magenta;
const receivedColor = has256 ? chalk.green.bgAnsi256(194) : chalk.green;

@quantizor
Copy link
Contributor Author

quantizor commented Jun 28, 2019

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

@pedrottimark
Copy link
Contributor

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

@quantizor
Copy link
Contributor Author

quantizor commented Jun 28, 2019

@pedrottimark what about something like this? (see the red notes for what I wasn't able to change in the screenshot)

60356560-4d76ec80-999f-11e9-8082-b1bf9c262f96

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.

@pedrottimark
Copy link
Contributor

Here is another variation:

  • Snapshot: magenta (same as recent pictures, instead of red)
  • Received: teal (instead of green)

2019-07-17 magenta teal

As before, goals include:

  • avoid exchanging meaning of green and red compared to reports for non-snapshot matchers
  • avoid opposite meaning of green and red compared to community tools for diff
  • avoid contrast between green and red because of accessibility
  • balance appearance different enough from other reports and similar enough to community tools?

The background received color in this version has hue 150° to give it a hint of green

Although teal has xterm number 6 it is not a color method in chalk package

That requires some more follow-up research to see if it achieves goals:

  • foreground colors have level 1 support in any terminal
  • background colors have level 2 support in most terminals

@pedrottimark
Copy link
Contributor

@probablyup Thank you for having brought this problem back to the front burner!

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

None yet

4 participants