-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addon-docs: Fix Preview scaling with transform instead of zoom #12845
Addon-docs: Fix Preview scaling with transform instead of zoom #12845
Conversation
There seems to be some typescript issue preventing a successful build. If those were fixed, I can take a look at the stories in chromatic. Do you need help? |
I think I got it under control! I'm going to do some refactoring to simplify the |
@Tomastomaslol it seems there's some bug in the zoom component when it comes to snapshotting it in chromatic, could you take a look? |
…oomed out. validate iframe id
…12324_zoom_buttons_in_docs_do_not_work
…an instead of div as componentwrapper
@ndelangen It looks like the zoomed out example was too "zoomed out" and caused the UI test to not run. I'm currently getting some unintentional diffs in UI tests. Hopefully, I will figure out a good solution for it soon 🙂 |
setIframeInnerZoom(scale: number) { | ||
try { | ||
if (browserSupportsCssZoom()) { | ||
Object.assign(this.iframe.contentDocument.body.style, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iframe.contentDocument can't work cross origin.. but I'm guessing addressing that is out of scope.
can we use useRef()
to get the current element instead though?
…nt to allow target css
…12324_zoom_buttons_in_docs_do_not_work
active?: boolean; | ||
}; | ||
|
||
export default class ZoomIFrame extends Component<IZoomIFrameProps> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit -- we've stopped using default exports in new code. Can you make these named exports instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great otherwise! 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes names exports are easier to trace and refactor 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be resolved! 🙂
…12324_zoom_buttons_in_docs_do_not_work
…omastomaslol/storybook into 12324_zoom_buttons_in_docs_do_not_work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
Hi @Tomastomaslol ! Can you check the Chromatic snapshots and see if they match your expectations? There are a bunch that look the same ... |
To me, it looks like the chromatic snapshots are picking up some changes to props. This PR shouldn't introduce any visual diff for chrome and to the best of my knowledge, there is nothing in the Chromatic test that would indicate that a visual diff has been introduced. Please let me know if there are any other scenarios you would like me to verify before we merge this code! 🙂 |
@Tomastomaslol I'm not talking about the args differences, I'm talking about the various zoom mode stories that you added. Do they look good to you? |
Hi @shilman! Sorry, I misunderstood what you were asking. Looking at the Iframe examples it doesn't look like the It possible it might be quite difficult to achieve a good example of the scaling levels due to the meta nature of this story. (Loading an example of itself into itself) and so far I have been unable to create any good examples demonstrating the scaling of the component. I will give it another go tomorrow with fresh eyes and see what I can come up with. Any suggestions or ideas on how to resolve this would be welcomed! 🙂 |
I have been spending some more time trying to figuring out why the iframe zoom examples didn't work. To me, it looks like the component gets recreated each time the controls are updated. because of that, the Zoom Iframe component will never set the scale: https://github.com/storybookjs/storybook/pull/12845/files#diff-b03e456200be1b58f1ba7f6b26405348ad11c73e5e7281b8601ff50c7b7dcb76R22 Example not using controls to set the scale: const TemplateIFrame = (args) => {
const iFrameRef = React.useRef<HTMLIFrameElement>(null);
const [scale, setScale] = useState(1);
return (
<div>
<input type="number" onChange={(e) => setScale(parseInt(e.target.value))} value={scale} />
<Zoom.IFrame iFrameRef={iFrameRef} scale={scale} active={true}>
<iframe
id="iframe"
title="UI Panel"
src="/iframe.html?id=ui-panel--default&viewMode=story"
style={style}
ref={iFrameRef}
allowFullScreen
/>
</Zoom.IFrame>
</div>
);
}; Results in: My current suggested solution applies the scale after the iframe content has loaded which make it possible to demonstrate the functionality within a story. The Zoom Iframe examples in chromatic look good now! Please let me know if this makes sense or if there is a better way to solve this issue! |
…12324_zoom_buttons_in_docs_do_not_work
Tested and it's looking great. Thanks for your patience on this @Tomastomaslol 🙏 |
Issue: #12324
What I did
zoom
withtransform
to be able to support component scaling in browsers that don't support CSSzoom
ref
around the previewed component to be able to calculate its heightChildrenContainer
based on the height of the previewed componenthttps://caniuse.com/?search=zoom
How to test