Skip to content

Changed toHaveStyle to use getPropertyValue instead of accessing the property directly #285

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Aug 11, 2020
Merged

Changed toHaveStyle to use getPropertyValue instead of accessing the property directly #285

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5c24818
Changed toHaveStyle to use getPropertyValue instead of accessing the …
luanraithz Jul 29, 2020
3966819
Using px instead of rem
luanraithz Jul 29, 2020
48ae84d
changed test description
luanraithz Jul 29, 2020
1c584c1
Update src/__tests__/to-have-style.js
luanraithz Jul 31, 2020
f2cb1e9
Update src/__tests__/to-have-style.js
luanraithz Jul 31, 2020
c13abf5
Update src/__tests__/to-have-style.js
luanraithz Jul 31, 2020
d5afec4
Update src/__tests__/to-have-style.js
luanraithz Jul 31, 2020
e54eff9
Update src/__tests__/to-have-style.js
luanraithz Jul 31, 2020
0d869d8
Apply suggestions from code review
gnapse Aug 11, 2020
e2b48ac
Merge branch 'master' into pr/getpropertyvalue
gnapse Aug 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/__tests__/to-have-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ describe('.toHaveStyle', () => {
expect(container.querySelector('.label')).toHaveStyle('color white'),
).toThrowError()

expect(() =>
expect(container.querySelector('.label')).toHaveStyle('--color: black'),
).toThrowError()
document.body.removeChild(style)
document.body.removeChild(container)
})
Expand All @@ -116,6 +119,33 @@ describe('.toHaveStyle', () => {
)
})

test('handles inline customer properties', () => {
const {queryByTestId} = render(`
<span data-testid="color-example" style="--color: blue">Hello World</span>
`)
expect(queryByTestId('color-example')).toHaveStyle('--color: blue')
})

test('handles global customer properties', () => {
const style = document.createElement('style')
style.innerHTML = `
div {
--color: blue;
}
`

const {container} = render(`
<div>
Hello world
</div>
`)

document.body.appendChild(style)
document.body.appendChild(container)

expect(container).toHaveStyle(`--color: blue`)
})

test('properly normalizes colors for border', () => {
const {queryByTestId} = render(`
<span data-testid="color-example" style="border: 1px solid #fff">Hello World</span>
Expand Down Expand Up @@ -170,6 +200,24 @@ describe('.toHaveStyle', () => {
})
})

test("doesn't consider the unit if it isn't explicitly set ", () => {
const {queryByTestId} = render(`
<span data-testid="color-example" style="font-size: 12rem">Hello World</span>
`)
expect(queryByTestId('color-example')).toHaveStyle({
fontSize: 12
Copy link
Member

Choose a reason for hiding this comment

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

Why would this match 12rem? If I set font-size: 12; then this results in 12px. This seems a bit confusing to me.

Also: This test only works in JSDOM. In an actual browser the value would be resolved to (depending on the base font size) to 240px.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! This test case doesn't make any sense as it is going to work only in JSDOM. Btw, I didn't know that, thanks! I'm going to change that.

It isn't clear to me if you are only against this test case or the idea of the api.

Copy link
Member

Choose a reason for hiding this comment

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

Just want to make sure that we're not documenting a test case that wouldn't work in a correct DOM environment.

Copy link
Member

Choose a reason for hiding this comment

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

Follow up on this. It seems from the discussion above that there's still something to be addressed here before this is ready. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Is JSDOM actually returning 12rem here has the computed style? Then all we have to do is add a comment explaining that this test depends on the user agent (since it will return the computed font-size in pixel which depends on the base font size for rem).

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 changed to px right after your first comment, do you think that is still worth to leave a note about that behavior?

})
})

test("Fails with an invalid unit", () => {
const {queryByTestId} = render(`
<span data-testid="color-example" style="font-size: 12rem">Hello World</span>
`)
expect(() => {
expect(queryByTestId('color-example')).toHaveStyle({ fontSize: '12px' })
}).toThrowError()
})

test('supports dash-cased property names', () => {
const {container} = render(`
<div class="label" style="background-color: blue; height: 100%">
Expand Down
2 changes: 1 addition & 1 deletion src/to-have-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function isSubset(styles, computedStyle) {
Object.entries(styles).every(
([prop, value]) =>
computedStyle[prop] === value ||
computedStyle[prop.toLowerCase()] === value,
computedStyle.getPropertyValue(prop.toLowerCase()) === value,
)
)
}
Expand Down