zeplin / storybook-zeplin

Storybook addon to view Zeplin resources in story panel
https://storybook-zeplin.netlify.app
MIT License
116 stars 16 forks source link

feat: Adds the option to use the design as overlay #13

Closed taltmann42 closed 3 years ago

taltmann42 commented 3 years ago

This PR adds a new panel that enables the user to show the design on top of the story-iframe. The overlay can be moved around with drag and drop to align with the elements of the story as needed. I started developing this locally and this addition helped tremendously with implementing pixel-perfect components. This is basically a direct integration of extensions like PerfectPixel

When the overlay is visible, multiple additional options are available:

Lock/Unlock

When the overlay is locked, pointer-events: none is set, so the story is interactive, and elements can be selected from the devtools directly through the design overlay. The overlay cannot be moved in this state.

Difference

Adds mix-blend-mode: difference to the overlay, so a pixel-by-pixel comparison can be seen. This addition was inspired by #2

Opacity Slider

With this slider, the opacity of the overlay can be controlled as needed.

Scaling Factor

When testing this solution with our designs, the overlay was twice as big as it should be - I guess the Design is using a different pixel density than the screen, so scaling the overlay down to x0.5 matched with how the components should look. If the 0.5 factor is an edge case, and x1 scaling is more common, the default should be adjusted (currently I kept the default to x0.5). Available options are

netlify[bot] commented 3 years ago

Deploy request for storybook-zeplin rejected.

Rejected with commit ab3759369c6cb1944669f3c5497ad153c753f4e3

https://docs.netlify.com/configure-builds/environment-variables/#sensitive-variable-policy

yannbf commented 3 years ago

Hey @taltmann42 what an amazing contribution!

I'm adding a gif just to make it easier for others to see it without having to run the repo. demo

yannbf commented 3 years ago

Thanks a lot for this contribution! I think it might be a bit different as to what @mertkahyaoglu mentioned in #2, given that this is more like pop-out from Zeplin rather than pixel diffing, but I love it!! It's gonna help lots of people. Maybe in the future we can have both pop-out and pixel diff.

A few observations: 1 - Your commits are authored as thomasaltmann@Thomass-MBP-4.fritz.box, and they're not tied to your user.. so you wouldn't get those contributions if this gets merged. I guess your git config user.email is not correct?

2 - It's currently a bit difficult to tell wether the icons are toggled on or off, especially the lock one. If possible it would be better to have a way to distinguish the states.. maybe change color when active. Like this: image

As reference, this is how Storybook shows active/inactive states for the addons in the toolbar: image

3 - There's a bug when the overlay gets out of bounds, I'm not sure if you noticed this edge case: bug

To reproduce: Click and drag the overlay until it's off the panel, then go back to the panel.

4 - The scale functionality is awesome, but sometimes the pixel density of the design and development might not match, even in the options 0.5/1/2. In the case of the Button above, the scale needed would be something like 0.24. There could be a case where the developer will try to play with scaling in the design from the panel + zooming the component, and that's just annoying. I'm not sure if it's the best idea to allow any custom scale, but might be pretty useful. If so, it also might be useful to have a way to set that custom scale via parameters, so that it's defined by default for every story and the users don't need to set it manually all the time. I guess this could be a future contribution if all parties find useful! :)

5 - The initial position of the overlay is always 0,0, but by default Storybook adds 16px padding to the stories, unless they have parameters.layout specified as 'centered' | 'fullscreen'. It would be nice to somehow figure that out and apply the padding in the initial overlay position, but it might be too much? I don't know, I guess it might be a nice to have.

If I'm not mistaken you can get access to params like this:

import { useParameter } from '@storybook/api';

const layout = useParameter('layout', 'padded'); // useParameter(parameterName, defaultValue)

That's all! What do you think? :)

mertkahyaoglu commented 3 years ago

@taltmann42 Wow. This is awesome! Great work. Thanks for the contribution. I'll take a look at the code whenever I got a chance.

@yannbf Thanks for the quick review. Yes, I agree. This is not what I thought while opening that issue but I love this too. It's like the pop out tool in Zeplin πŸ˜„ We should definitely have this!

taltmann42 commented 3 years ago

Hi @yannbf and @mertkahyaoglu thanks for the appreciation πŸ™‚

Regarding your observations:

  1. Yes, I enabled the verification with another email adress (also connected with my account - with its own GPG key) and it seems like all my commits are now signed with the other email address - can I somehow sign this commit again correctly?
  2. I'll look into it πŸ‘
  3. This doesn't seem to be connected to leaving the viewport but quick mousemovements in general. If you move the cursor quick enough, so it would be outside the image in the next frame, the mousemove event doesn't fire anymore because you're not over the image anymore. A workaround would be to add the mousemove listener to the #storybook-zeplin-preview element and make it absolute and fill the iframe-area. As long as the overlay isn't being dragged, the element would need a pointer-events: none, otherwise the story wouldn't be interactive anymore - what do you think of that approach?
  4. Making the default scaling factor configurable sound great - as for the custom scaling: I could add a text/number input which controls the scaling. The dropdown could then just be removed completely or serve as a "quickselect" option. Alternatively - why not just make the array of scaling-options configurable? I doubt that the designs would differ in the pixel densities that much in the same project, but if it's known that there may be 2 or 3 specific cases, these can be configured and loaded into the dropdown. Also - when only a single option is configured, we could not display the dropdown at all and save some space.
  5. Switching between 0,0 and 16,16 shouldn't be that big of a problem if I can get the layout options. However, if the 16px can be configured as well, or overwritten with custom styling, it might still be off - in that case, the only way I can think of right now would be to get the contentDocument of the iframe and measure the padding of the element .sb-main-padded/sb-show-main, which I'm not a big fan of tbh.
yannbf commented 3 years ago
  1. Yes, I enabled the verification with another email adress (also connected with my account - with its own GPG key) and it seems like all my commits are now signed with the other email address - can I somehow sign this commit again correctly?

I believe the easiest way to fix this is by running this in the commit you need (You will need to force push the branch, as the commit hash will change.):

git commit --amend --no-edit --author="Your Name <your@email.com>"

If you have multiple commits, you can do an interactive rebase and go through them:

1 - git rebase --interactive HEAD~X // where X is the number of commits you want to go through
2 - then change from pick to edit all the commits you want to change
3 - git commit --amend --no-edit --author="Your Name <your@email.com>"
4 - git rebase --continue // then redo step 2 till 4 until you're done

If you want to change your email in the git config so this issue does not happen anymore:

git config user.email "yourcorrect@email.com" // in case you want to apply it for the current project
git config --global user.email "yourcorrect@email.com" // in case you want to apply this globally
  1. This doesn't seem to be connected to leaving the viewport but quick mousemovements in general. If you move the cursor quick enough, so it would be outside the image in the next frame, the mousemove event doesn't fire anymore because you're not over the image anymore. A workaround would be to add the mousemove listener to the #storybook-zeplin-preview element and make it absolute and fill the iframe-area. As long as the overlay isn't being dragged, the element would need a pointer-events: none, otherwise the story wouldn't be interactive anymore - what do you think of that approach?

I see! now it's quite easy to reproduce, I just need to click a the corner of the overlay and move it a little bit. You can try experimenting with that solution for sure, just have to be a bit careful not to impact the experience of the users when interacting with stories. In that solution, I might be wrong but I believe the element would always be placed on top of the story, even if the addon is not selected. One example of a problem that could happen is.. if for some reason the pointer events are not set properly and the user navigates to another story, they might be stuck not being able to interact with the story, and most specially, they might think it's a bug in Storybook rather than in the addon.

  1. Making the default scaling factor configurable sound great - as for the custom scaling: I could add a text/number input which controls the scaling. The dropdown could then just be removed completely or serve as a "quickselect" option. Alternatively - why not just make the array of scaling-options configurable? I doubt that the designs would differ in the pixel densities that much in the same project, but if it's known that there may be 2 or 3 specific cases, these can be configured and loaded into the dropdown. Also - when only a single option is configured, we could not display the dropdown at all and save some space.

That sounds nice! We'd just need to check how good the UX will be and make sure to document that well! If you want to go with this solution, most likely we would need to add a preset to this addon (so that it already provides parameter values out of the box). Let me know in case you'd like assistance!

  1. Switching between 0,0 and 16,16 shouldn't be that big of a problem if I can get the layout options. However, if the 16px can be configured as well, or overwritten with custom styling, it might still be off - in that case, the only way I can think of right now would be to get the contentDocument of the iframe and measure the padding of the element .sb-main-padded/sb-show-main, which I'm not a big fan of tbh.

Yea I thought of that.. usually people go with the layout parameter (which only has limited options and cannot be overwritten), but some people might add custom stylings in their preview-head.html file and there they can do whatever they want. Maybe this is tricky and we should just forget about it :D

taltmann42 commented 3 years ago
  1. That worked, thank you!
  2. Icons now stay in their active state when toggling
  3. There is now an element which is only rendered when the overlay is actively being moved, so it shouldn't interfere with anything else. Since the iframe beneath the image is not part of the parent window, the mousemove event didn't fire anymore.
  4. I'm not sure how we should use a preset - the docs mention that global parameters are a common way to configure addons: https://storybook.js.org/docs/react/writing-stories/parameters#global-parameters - we could then access these parameters with the useParameter hook and some sensible defaults (feel free to suggest which values you'd like to have)
  5. I think forgetting about it for now is a good option, as there is also the centered layout which would also need some special handling
yannbf commented 3 years ago
  1. I'm not sure how we should use a preset - the docs mention that global parameters are a common way to configure addons: https://storybook.js.org/docs/react/writing-stories/parameters#global-parameters - we could then access these parameters with the useParameter hook and some sensible defaults (feel free to suggest which values you'd like to have)

Yea, so we have two ways to deal with values in parameters:

1 - Add sensible defaults when using useParameters, as you mentioned 2 - Adding a preset. A preset file will automatically run when the addon is registered, and there you can add default values for the parameters. You can get some info on how to do it in the Preview entries section of the docs

yannbf commented 3 years ago

Hey @taltmann42 just wanted to say I tested it out with your new changes and it works really great!

And also notify @mertkahyaoglu, although you probably already know, that Storybook 6 now uses a caching mechanism for the manager, so if you need to rebuild the addon, after you do so, you should use yarn storybook --no-manager-cache to make sure your changes are picked up!

mertkahyaoglu commented 3 years ago

And Netlify preview is ready πŸ”₯ https://604d05ed53c698a0b8efea34--storybook-zeplin.netlify.app/

yannbf commented 3 years ago

One little issue though. If addon panel is on the right side, inputs are not visible. Maybe we might make opacity input smaller or move them below the buttons. I'm not sure we should support it though. What do you guys think? @taltmann42 @yannbf

I think if people are using this addon and their panel is on the right (I prefer to use it like that), they will have it more expanded than that.. so it's not really a problem. The problem occurs if the panel is really small, and in that case people wouldn't even be able to see the full design. I think it's OK but it could also be more responsive

yannbf commented 3 years ago

Hey @taltmann42 @mertkahyaoglu I think this is 99% good to go!

I believe we should really keep 0.5 as default scaling factor. I tried other scenarios: Exported design from Figma and exported design from Sketch.

From figma at 0.5x scaling factor: image

From figma at 1x (hiding sidebar and addons panel wasn't enough to fit in!): image

From sketch at 0.5x scaling factor: image

Other than that it's perfect πŸ‘Œ

mertkahyaoglu commented 3 years ago

@taltmann42 Awesome work! Merging this then.

yannbf commented 3 years ago

@taltmann42 Awesome work! Merging this then.

Nice! let me know once you make the release so I can start testing in my projects πŸ‘―

mertkahyaoglu commented 3 years ago

@taltmann42 @yannbf Zeplin would like to send you some swag for your contributions πŸŽ‰ I can see Yann's email in his profile but @taltmann42 I couldn't find yours. Could you send me an email?