oblador / loki

👁 Visual Regression Testing for Storybook
https://loki.js.org/
MIT License
1.75k stars 171 forks source link

selector fails if a story returns null #45

Open gvaldambrini opened 6 years ago

gvaldambrini commented 6 years ago

First of all, thanks for your wonderful addon! I found a problem with empty components. If a component returns null and a story renders it, loki fails with this error:

Unable to get position of selector "#root > *". Review the `chromeSelector` option and make sure your story doesn't crash.
oblador commented 6 years ago

Yup, it can't find any element to screenshot. Why would you have a story that renders null though?

gvaldambrini commented 6 years ago

In my case it's because I have a simple component that doesn't have any data to render (and for the sake of simplicity I don't want to put a loader). Regardless my use case imo this is an issue to fix because a component that renders null is a valid React component (and so I expect that Loki can handle it).

oblador commented 6 years ago

So in that case it would be a 0x0 image? I would say that in most cases a story not rendering anything is probably due to an error.

gvaldambrini commented 6 years ago

Rendering an image 0x0 would be fine. If you think that in most cases a story not rendering anything hid an error, would be printing a warning a solution for everything?

rwieruch commented 4 years ago

I ran into the same issue when using a React Portal. For instance, a Dialog is just rendered with a Portal:

const Dialog = ({
  size,
  onDocumentClick,
  hasBlurryOverlay,
  children,
  ...rest
}) =>
  ReactDOM.createPortal(
    <>
      {hasBlurryOverlay && <BlurryOverlay onClick={onDocumentClick} />}
      <DialogContainer size={size} {...rest}>
        {children}
      </DialogContainer>
    </>,
    document.body,
  );

I got around it by just rendering the Dialog content without the Portal for the visual testing story.

Thanks for this project @oblador 🚀

levymetal commented 4 years ago

I think there are legitimate use cases for this. Take the following component, an accessibility label designed for screen readers. We don't know what the implementation is; only that nothing should be visible.

<AccessibilityLabel content="Only screen readers can see me" />

If this component starts rendering something, it's a visual regression. Using loki to test for such a regression seems just as valid as any other use case.

Another example could be a component that conditionally renders based on a media query and you're using Loki with multiple configurations, one of which results in a null render.

We've solved this by using a decorator that wraps the story in a minimum 1x1 div, but it would be delicious if it were baked into loki.

donaldpipowitch commented 3 years ago

just rendered with a Portal

Does loki support portals at all? I have the feeling what is rendered by a Portal will never be rendered by loki 🤔 But I might need to verify this.

techeverri commented 3 years ago

I have the feeling what is rendered by a Portal will never be rendered by loki

Loki does not render anything. It takes a screenshot of an element.

donaldpipowitch commented 3 years ago

Loki does not render anything. It takes a screenshot of an element.

Sorry for the bad phrasing. But yeah, thinking about It takes a screenshot of an element. probably reveals my problem. I thought by default loki takes a screenshot of my page not a single element. I couldn't find it in the docs, but I found the default settings in the source code: https://github.com/oblador/loki/blob/317c2c976dbe5f45a9a1e29fe03d816835231e6a/packages/runner/src/commands/test/default-options.json#L11

It takes a screenshot of #root > * by default. My portal is rendered into body. Maybe that's why I can't see it.

levymetal commented 3 years ago

We do something like this to test portaled content:

export const story = () => {
  document.getElementById('root').insertAdjacentHTML('afterbegin', '<div class="portal" />');
  return ReactDOM.createPortal(<div>meow</div>, document.querySelector('.portal'));
};

You can't simply render the portal container with JSX as the node won't be present in the DOM during the initial render.

With this is mind, I think you're better off having two components, one that renders the component (eg, a named export) and another that wraps it in a portal (default), then use loki to test the named export.

techeverri commented 3 years ago

I thought by default loki takes a screenshot of my page not a single element. I couldn't find it in the docs, [...]

The docs for the configuration mentions the selector and how Loki uses it 👇 @donaldpipowitch

Configuration

[...]

chromeSelector

This setting is a CSS selector to the part of the page you want screenshots of. This is useful if you have decorators that's not really part of the component itself. Note that it doesn't screenshot the DOM element itself but rather the crops the screenshot to those dimensions, so if you have any elements absolutely positioned above they will be included.

🔗 https://loki.js.org/configuration.html#chromeselector

donaldpipowitch commented 3 years ago

Sorry for the bad phrasing again. Yes, the chromeSelector is documented, but not the default value.

techeverri commented 3 years ago

Since the default value is for one of the command line arguments of the loki test command, it is documented in the "Command line arguments" section (check the table)

🔗 https://loki.js.org/command-line-arguments.html

rshackleton commented 2 years ago

Loki does not render anything. It takes a screenshot of an element.

Sorry for the bad phrasing. But yeah, thinking about It takes a screenshot of an element. probably reveals my problem. I thought by default loki takes a screenshot of my page not a single element. I couldn't find it in the docs, but I found the default settings in the source code:

https://github.com/oblador/loki/blob/317c2c976dbe5f45a9a1e29fe03d816835231e6a/packages/runner/src/commands/test/default-options.json#L11

It takes a screenshot of #root > * by default. My portal is rendered into body. Maybe that's why I can't see it.

I was having the same issue. I've updated the code to render into #root if available otherwise it uses the body.

const portalTarget = document.getElementById('root') ?? document.body;