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

Implement snapshot zooming and panning, sliding diff and blend-mode for diff overlay #151

Closed
wants to merge 4 commits into from

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 29, 2023

Screen.Recording.2023-11-29.at.14.56.09.mov
📦 Published PR as canary version: 0.0.123--canary.151.7ff0e88.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.123--canary.151.7ff0e88.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.123--canary.151.7ff0e88.0

@ghengeveld ghengeveld changed the title Implement snapshot zooming and padding, sliding diff and blend-mode for diff overlay Implement snapshot zooming and panning, sliding diff and blend-mode for diff overlay Dec 4, 2023
Copy link
Member

@thafryer thafryer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Largely, this looks good. I would ask that we find some way to clarify any complexity that exists in the calculations for the various styles. On first glance, it's a tough to crack the code without a deeper level of investigation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what's really happening here without a lot of experimentation to determine why we calculate some of these values in this way. The question below is what I asked myself looking at this component and I was not able to answer it confidently. I did have some guesses.

If someone else (or you 6 months later) needed to adjust this component, what would they be adjusting and why?

@domyen
Copy link
Member

domyen commented Dec 5, 2023

I think this is really rad Gert.

General feedback
It was tough to tell whether I was looking at the latest snapshot or the baseline snapshot on hover. It's made clear when you zoom in, but on the surface I wasn't sure what I was looking at.

Suggestion:

  • Perhaps you could label each side of the line by default? Not sure if this is more obvious in context of the Storybook UI.

Feedback on line
It was tough to tell what's happening on hover with the line because it seems to use the background color of the ground (white and dark respectively). This was confusing because I thought it may have been an UX artifact or bug.

Suggestion:

  • Make the line obvious and red #FF440070. This is similar to what tools like Datadog do when you hover over the graphs.
  • Remove the shadow (because the red makes the line super pronounced)

Feedback on diff blend mode
I was confused by what the difference is between dark green and light green. So when I was moving my cursor left and right to reveal more UI, I couldn't tell what was different.

Suggestion:

  • Show the regular image (without the diff) when the baseline is in view. Show the diff for the "latest" snapshot on the right hand side.

@MichaelArestad
Copy link
Contributor

@domyen I tried a higher contrast line in a prototype I worked up last week. Here's a quick video of it.

Screenshare.-.2023-11-29.11_47_34.AM.mp4

@ghengeveld I agree with Dom that the line is hard to see. I would do something like this when the user clicks on the drag line:
image

@domyen
Copy link
Member

domyen commented Dec 5, 2023

That higher contrast grey line looks good to me @MichaelArestad

In Gert's demo, the interaction is on hover not drag (clicking zoom) so we wouldn't be able to see the blue like you have above. I suspect this is OK and you already know about that, but wanted to call it.

@MichaelArestad
Copy link
Contributor

@domyen Yeah. Since we started talking about hover interactions like these (and the zooming), I got to thinking about them a bit more. Something about them doesn't sit quite right with me. It almost feels like I don't have full control over what I want to see. For example, when my cursor enters from one side or the other, the bar goes to where my cursor is, which isn't super useful or intentional. Also, I may want to leave it split a certain way which requires I exit the cursor area from the top or bottom. I know I prefer to click to drag because it feels like I'm in control of what I'm looking at. That said, I'm not overly opinionated on this quite yet and would like to see how others like it.

@domyen
Copy link
Member

domyen commented Dec 6, 2023

Yea that's fair re:not feeling in control over what I see. I think you articulated what I was responding to here:

It was tough to tell whether I was looking at the latest snapshot or the baseline snapshot on hover. It's made clear when you zoom in, but on the surface I wasn't sure what I was looking at.

I suspect it'd make sense behind a click like our other diff tools.

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

Successfully merging this pull request may close these issues.

None yet

4 participants