-
Notifications
You must be signed in to change notification settings - Fork 14
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
test(BackgroundMedia): test for BackgroundMedia added #170
Conversation
84ec579
to
0286995
Compare
Preview is ready. |
e0860aa
to
aff2faf
Compare
src/components/Media/Image/Image.tsx
Outdated
@@ -78,7 +80,7 @@ const Image = (props: ImageAllProps) => { | |||
const imageBackground = (oneImage: ImageProps) => { | |||
const imageData = getMediaImage(oneImage); | |||
return ( | |||
<animated.div style={{transform: parallaxInterpolate}}> | |||
<animated.div style={{transform: parallaxInterpolate}} data-qa={qaIdOfAnimatedDiv}> |
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.
Don't we need custom qa here too?
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.
Now, it seems to me that we do not need custom ones
I cannot see necessity to pass a bunch of qaIds so deep in a component.
What we can do here is add a custom role to <animated.div />
<animated.div role="animated-div" />
By this way in a test we can access this element like
screen.getByRole('animated-div') // instead of screen.getByTestId(qaIdOfAnimatedDiv)
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.
UPDATE: Have talk to Andrey. Adding role is not good idea. Describe it test's readme
aff2faf
to
6654cfb
Compare
expect(component).toHaveStyle(style); | ||
}); | ||
|
||
test('render video with defined height', () => { |
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.
maybe custom instead of defined?
Here and in other places
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.
sure
src/components/Media/Video/Video.tsx
Outdated
<div | ||
className={b('wrap', videoClassName)} | ||
style={{height}} | ||
data-qa={defaultMediaVideoQa} |
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 saw in other places qa || defaultMediaVideoQa
Should we add here custom qa too?
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.
Slightly changed common way we assert qa attributes. Thank you for point on this pattern
872ca66
to
491d1f1
Compare
5081b1f
to
e22eeee
Compare
No description provided.