Skip to content

Conversation

@shlomitc
Copy link
Contributor

No description provided.

Change your `applitools.config.js` file to the following:
```js
module.exports = {
waitBeforeScreenshots: `[data-test-ready="true"]`,
Copy link

@jonathanadler jonathanadler Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I actually created a dataHooks file for this.
I thought that users could import the datahook from the file, and use it

const { DATA_READY_HOOK } = require('storybook-snapper/dist/src/dataHooks');
module.exports = {
  waitBeforeScreenshots: `[${DATA_READY_HOOK}="true"]`,
  ...
}

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this, it's internal implementation and import is ugly. We should provide default configuration with some way to override it

Copy link

@jonathanadler jonathanadler Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what you're saying is that you want to have eyes-storybook as an internal implementation of this library?
And maybe add a binary that will run eyes-storybook?
And users will be able to add a config file with overrides?

So all configuration will be done in one place?

Hmmm 🤔
I think I like the idea.
So no one will really need to deal with eyes-storybook themselves, and in the future, we might be able to change applitools for something else if we want.

wdyt?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the case, then we might need to change the name to something without storybook (although if we ever use something else than storybook, consumers will have their tests be broken)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapping eyes-storybook sounds a bit too much at the moment. I actually thought exposing the storybook config instead like storybook's webpack config extension works.

const {storybookSnapperApplitoolsConfig} = require('storybook-snapper');
module.exports = storybookSnapperApplitoolsConfig({
  //rest of the configuration
});

Copy link

@jonathanadler jonathanadler Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
I usually don't like having multiple "moving parts", but I guess as a start it's a good way to go

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, what I initially wrote is also "moving parts", so I guess your approach will be better for now

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

Successfully merging this pull request may close these issues.

2 participants