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

Consider removing prop spreading from VRT output #875

Open
montezume opened this issue Jun 19, 2019 · 5 comments
Open

Consider removing prop spreading from VRT output #875

montezume opened this issue Jun 19, 2019 · 5 comments

Comments

@montezume
Copy link
Contributor

Summary

Right now, we show all of the props of the direct child of a Spec in our visual regression tests.

const Props = props => {
  const node = React.Children.only(props.children);
  const propEntries = Object.entries(node.props);
  return (
    <PropList>
      {propEntries
        .filter(([, value]) => typeof value !== 'function')
        .map(([key, value]) => (
          <Pill key={key} label={key} value={value} />
        ))}
    </PropList>
  );
};

This leads to visual output like this.

Screen Shot 2019-06-19 at 10 55 10 AM

Problems with this

While this was useful at the start for reviewing the initial baseline VRTs, it causes problems when we refactor our components. If you check this PR, you will see that we didn't make any changes to the props accepted by the component. All we did was remove a surrounding HOC around the component. However, the following visual diff showed up

Screen Shot 2019-06-19 at 10 57 05 AM

Suggestion approach

Remove display of props, and use the Spec label to properly display the component state.

@jonnybel
Copy link
Contributor

I think it would be nice if we could just display the props we wanted in the VRT, instead of all of them or none of them.

For instance, on this simple Toggled example, just pass the isToggled prop (and perhaps also the isToggleButton) to make it clear which are the props that cause the visual difference.

@montezume
Copy link
Contributor Author

Yeah that would make sense as well. 👍

@emmenko
Copy link
Member

emmenko commented Oct 8, 2020

@jonnybel what should we do about this?

@tdeekens
Copy link
Contributor

tdeekens commented Oct 9, 2020

Feels to me, out of the blue, like we could add a hideProps so we can by default show all props but hide specific ones. We could also have a showProps depending on what's more useful maybe.

@jonnybel
Copy link
Contributor

jonnybel commented Oct 9, 2020

Currently, we can either omit all props with omitPropsList and we can filter which props do display with propsToList, but most of the VRTs are still not using it.

I'm not sure if it's worth to go through all VRTs and make them to use the filter. I guess we should try to use the filter when we refactor components and their VRT that show a diff in the props list, and also to newly added VRTs.

And would it be worth to also have a propsToHide (the opposite of propsToList)?

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

No branches or pull requests

4 participants