Extend functionality of the toHaveStyleRule matcher #1110
Conversation
…tion into jest-emotion-extend-matcher
Werter12
changed the title
Codecov Report
|
|
|
||
| it('matches styles on the nested component or html element', () => { | ||
| const Svg = styled('svg')` | ||
| width: 100%; |
There was a problem hiding this comment.
If I were to set a color here, fill: blue;, which is overridden later to fill: green; and the test checks for 'blue' will the tests still pass even though the end visual result is a green fill?
There was a problem hiding this comment.
The test check for fill: blue will fail. The fill would be green.
There was a problem hiding this comment.
Could you add a test for that?
this was a reply to #1110 (comment) but GitHub shows replies from reviews strangely
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't think that tests it, that looks like it's only declaring the fill property once.
There was a problem hiding this comment.
@mitchellhamilton lets have a chat in slack, I have sent you a message there
| .reduce((decs, rule) => Object.assign([], decs, rule.declarations), []) | ||
| let preparedRules = styles.stylesheet.rules | ||
| if (media.length > 1) { | ||
| if (shortMediaRegExp.test(media) || longMediaRegExp.test(media)) { |
There was a problem hiding this comment.
Will longMediaRegExp break when more than one and clause is preset?
@media screen and (min-width: 30em) and (orientation: landscape)
There was a problem hiding this comment.
Just checked this scenario and it is passed. But I think we can get rid of longMediaRegExp because it is don't extend shortMediaRegExp properly and a invalid media can pass this check by adding () and () pattern
| /** | ||
| * RegExp for long media statement with and | ||
| * @type {RegExp} | ||
| */ |
There was a problem hiding this comment.
Could you remove these comments? They aren't really helpful since you can see that it's a regex and the variable name says what it's for.
packages/jest-emotion/src/utils.js
Outdated
| +media: string; | ||
| +rule: string; | ||
| } | ||
| export const RULE_TYPES: IRuleType = { |
There was a problem hiding this comment.
Could you remove this interface? flow can infer what the type is so it's not necessary and it doesn't really help anything.
| .toJSON() | ||
|
|
||
| expect(tree).toHaveStyleRule('color', 'yellow', { target: 'hover' }) | ||
| expect(tree).toHaveStyleRule('color', 'black', { target: 'focus' }) |
There was a problem hiding this comment.
These targets seem confusing, why are they hover and focus, as a consumer I would expect target to accept a css selector so I would expect this to target html elements named hover and focus.
There was a problem hiding this comment.
I agree I will replace it on ':hover' and ':focus'
| if (target === '') { | ||
| return classNames.includes(selector.slice(1)) | ||
| } | ||
| return selector.includes(target) |
There was a problem hiding this comment.
Could you add a comment here explaining what's happening?
There was a problem hiding this comment.
I've added comments in the code what is going on in this function. Should I add comments here in discussion?
| value: *, | ||
| options?: { target?: string, media?: string } = {} | ||
| ) { | ||
| const { target = '', media = '' } = options |
There was a problem hiding this comment.
There's no need for the default values here, leave them as undefined and handle undefined instead of an empty string where necessary.
…RuleType interface
…eft one media regexp for checks
| @@ -23,6 +26,8 @@ function isAsymmetric(obj) { | |||
| return obj && isA('Function', obj.asymmetricMatch) | |||
| } | |||
|
|
|||
| const mediaRegExp = /\([a-z-]+:\s[a-z0-9.]+\)/ | |||
There was a problem hiding this comment.
Would this work with @media only screen or something else like that?
There was a problem hiding this comment.
@mitchellhamilton I've change regexp to make media with screen property pass
There was a problem hiding this comment.
I didn't just mean screen, it was an example, I meant all the media types(https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries#Media_types) and I'd prefer that the media types aren't hardcoded.
|
|
||
| it('matches styles on the nested component or html element', () => { | ||
| const Svg = styled('svg')` | ||
| width: 100%; |
There was a problem hiding this comment.
Could you add a test for that?
this was a reply to #1110 (comment) but GitHub shows replies from reviews strangely
| .reduce((decs, rule) => Object.assign([], decs, rule.declarations), []) | ||
| let preparedRules = styles.stylesheet.rules | ||
| if (media) { | ||
| if (mediaRegExp.test(media)) { |
There was a problem hiding this comment.
I don't really get the point of this, why do we need to check if the media query is valid at all? jest-emotion is really for stopping regressions so people should test components in a browser so they'll know if their media query is valid or not. I'd rather remove the check so that we don't have to maintain that regex.
|
Also could you add a test like this let tree = renderer
.create(
<div
className={css`
color: green;
color: hotpink;
`}
/>
)
.toJSON();
expect(tree).not.toHaveStyleRule("color", "green");
expect(tree).toHaveStyleRule("color", "hotpink"); |
Werter12
force-pushed
the
jest-emotion-extend-matcher
branch
from
ddf7a03
to
d92fe2b
Compare
What and Why:
Fixed bug #1109 . Added additional param
optionsfor toHaveStyleRule matcher with propertiestargetandmediawhich will help developers to test their components with better accessibility (the style rule will be more accessible for testing).target- will help to access style rule inside hover, focus or to nested html element.media- will help to access css rule in the media queries.Checklist: