chromaui / addon-visual-tests

Visual Tests addon for Storybook
MIT License
31 stars 2 forks source link

Move switch and diff buttons #139

Closed ghengeveld closed 9 months ago

ghengeveld commented 9 months ago

Telescoping on https://github.com/chromaui/addon-visual-tests/pull/138

📦 Published PR as canary version: 0.0.117--canary.139.93bc623.0
:sparkles: Test out this PR locally via: ```bash npm install @chromaui/addon-visual-tests@0.0.117--canary.139.93bc623.0 # or yarn add @chromaui/addon-visual-tests@0.0.117--canary.139.93bc623.0 ```
weeksling commented 9 months ago

Not specifically PR feedback in need of change, but just some general design feedback.

I find this orientation quite disorienting. I think I like the toggle on the bottom, but it feels like there is a lot of content on the top bar now.

Screen Shot 2023-11-03 at 5 33 59 PM

That was without changes. With changes, it shifts around a bit because of the line break. Though I am really glad that it linebreaks because we can see the entire branch name. I wonder if the snapshot toggle could live on the bottom of the screen, above the browser and mode selectors, and then keep the top line for controls. @MichaelArestad @ghengeveld After using it to review this pr, having the baseline toggle, diff control, and accept button the same line was super fast. So maybe ignore this feedback.

Screen Shot 2023-11-03 at 5 38 36 PM
ghengeveld commented 9 months ago

@weeksling Thanks for the thoroughness. I fixed the diff toggle bug in the upstream PR.

As for the design, I'm working on some improvements in another ticket. The switch button is going to move to the right next to the diff toggle. The accept buttons are moving up above those diff controls.

I'm truncating (with ellipsis) the branch name. That's on just a single line though. Should we allow wrapping to a second line @MichaelArestad? That would be a bit more complicated (it's not a native CSS feature) but seems like a good idea.

MichaelArestad commented 9 months ago

Should we allow wrapping to a second line

No. Let's keep it to a single line for now for simplicity in both design and implementation.

ghengeveld commented 9 months ago

Closing in favor of #148