-
Notifications
You must be signed in to change notification settings - Fork 3
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
explain how to make async tests work #4
base: master
Are you sure you want to change the base?
Conversation
Change your `applitools.config.js` file to the following: | ||
```js | ||
module.exports = { | ||
waitBeforeScreenshots: `[data-test-ready="true"]`, |
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.
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?
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.
I don't like this, it's internal implementation and import is ugly. We should provide default configuration with some way to override it
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.
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?
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.
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)
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.
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
});
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.
I see.
I usually don't like having multiple "moving parts", but I guess as a start it's a good way to go
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.
I mean, what I initially wrote is also "moving parts", so I guess your approach will be better for now
No description provided.