Skip to content
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

Jump to bottom

fix: support uppercase custom props in toHaveStyle #552

Merged
merged 4 commits into from
Nov 30, 2023
Merged

fix: support uppercase custom props in toHaveStyle #552

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b12b4f4
fix: support uppercase custom props in toHaveStyle
depoulo Nov 28, 2023
e7aee18
fix: make the test pass
depoulo Nov 28, 2023
118f295
fix: handle different use cases
depoulo Nov 29, 2023
90296fc
fix: turned out to be a little more complex
depoulo Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/__tests__/to-have-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,11 @@ describe('.toHaveStyle', () => {
)
})

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

test('handles global custom properties', () => {
Expand Down Expand Up @@ -205,7 +205,7 @@ describe('.toHaveStyle', () => {
<span data-testid="color-example" style="font-size: 12px">Hello World</span>
`)
expect(queryByTestId('color-example')).toHaveStyle({
fontSize: 12
fontSize: 12,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

})
})

Expand All @@ -214,7 +214,7 @@ describe('.toHaveStyle', () => {
<span data-testid="color-example" style="font-size: 12rem">Hello World</span>
`)
expect(() => {
expect(queryByTestId('color-example')).toHaveStyle({ fontSize: '12px' })
expect(queryByTestId('color-example')).toHaveStyle({fontSize: '12px'})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one came from the husky pre-commit hook formatting the whole file. By the way running npm run format would have changed a bunch of other files, too.

}).toThrowError()
})

Expand Down
1 change: 1 addition & 0 deletions src/to-have-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function isSubset(styles, computedStyle) {
Object.entries(styles).every(
([prop, value]) =>
computedStyle[prop] === value ||
computedStyle.getPropertyValue(prop) === value ||
computedStyle.getPropertyValue(prop.toLowerCase()) === value,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you make it so that we keep the lower case transformation when the prop does not look like a custom css prop?

After reading the whole discussion here, I thought this would be the least disruptive way.

Copy link
Member

Choose a reason for hiding this comment

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

I still think that we may need to detect specifically if the prop name corresponds to a custom CSS property. They are case-insensitive. The way the code is right now, if I have an element with --var-name: 0px, a test checking for --VAR-NAME: 0px would pass, and that's not accurate.

Can't you do something along the lines of:

const isCustomProp = prop.startsWith('--')
if (!isCustomProp) {
  prop = prop.toLowerCase()
}
// Keep the condition as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my latest commit

)
)
Expand Down