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

Flip magenta and yellow? Change colors back to red/green? #39

Open
mcmire opened this issue Sep 16, 2019 · 7 comments
Open

Flip magenta and yellow? Change colors back to red/green? #39

mcmire opened this issue Sep 16, 2019 · 7 comments

Comments

@mcmire
Copy link
Collaborator

mcmire commented Sep 16, 2019

When printing:

Expected: ...
       to eq: ...

The "actual" value is in red and the "expected" value is in green. Shouldn't it be the other way around? (i.e. the actual value should be assumed to be the right one).

Note that this means we'd have to flip the colors in the diff as well, and update the legend.

@mcmire mcmire changed the title Colors are flipped? Flip red and green? Sep 16, 2019
@mcmire
Copy link
Collaborator Author

mcmire commented Sep 18, 2019

Not sure why I got confused here. It already works like I thought it did.

@mcmire mcmire closed this as completed Sep 18, 2019
@mcmire mcmire changed the title Flip red and green? Flip magenta and yellow? Oct 10, 2019
@mcmire
Copy link
Collaborator Author

mcmire commented Oct 10, 2019

Reopening this. It feels like more often than not, the actual value is more important than the expected value, so it should jump out more when looking at the output. This will cause the diff to look weird, though, so maybe not?

@mcmire
Copy link
Collaborator Author

mcmire commented Dec 10, 2019

This is what Jest does, for reference:

Screen Shot 2019-12-09 at 9 33 11 PM

@mcmire
Copy link
Collaborator Author

mcmire commented Dec 11, 2019

Maybe worth looking at Ava too: https://github.com/avajs/ava#magic-assert

@mcmire mcmire changed the title Flip magenta and yellow? Flip magenta and yellow? Change colors back to red/green? Jan 26, 2020
@hedgesky
Copy link

Would it make sense to make this configurable? Thus, the cost of the decision for the question in the title wouldn't be that high, because end users would be able to adjust color scheme to their preference if they'd like to.

For example, something like this:

SuperDiff.configure do |config|
  config.set_color_scheme(SuperDiff::ColorSchemes::RedGreen) 
  # or MagentaYellow, or YellowMagenta
end

With schemes defined either similar to as they do now:

module MagentaYellow
  COLORS = {
    alpha: :magenta,
    beta: :yellow,
    border: :blue,
    header: :white,
  }.freeze
end

or without relying on hashes:

module MagentaYellow
  def self.alpha
    :magenta
  end

  def self.beta
    :yellow
  end

  def self.border
    :blue
  end

  def self.header
    :white
  end
end

As another option, set_color_scheme could accept symbol instead of a module as an argument. That approach would be less verbose, but less extensible.

@mcmire
Copy link
Collaborator Author

mcmire commented Nov 20, 2020

Yeah, that's a good point, perhaps the user is the best judge here. I like the idea of passing a module. Or perhaps just a hash?

SuperDiff.configure do |config|
  config.color_scheme = SuperDiff::ColorSchemes.red_green
  # or magenta_yellow, or yellow_magenta... or supply your own
end

or if you wanted to change a couple of settings on the fly, you could even do:

SuperDiff.configure do |config|
  # color_scheme is set to red_green by default
  config.color_scheme.merge!(alpha: :magenta, beta: :yellow)
end

@hedgesky
Copy link

Just to note, I suggested .set_color_scheme method name as it (for my taste, at least) is a bit more aligned with . add_extra_differ_class and other already used configuration methods.

Regarding passing hashes and encouraging users to merge values into them: it's nice and simple solution. A downside: users could rely on the fact that it's a hash, making major changes to color scheme functionalities more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants