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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix negated ".not" modifier usage edge case #210

Merged
merged 3 commits into from Nov 11, 2018
Merged

Fix negated ".not" modifier usage edge case #210

merged 3 commits into from Nov 11, 2018

Conversation

santino
Copy link
Contributor

@santino santino commented Nov 10, 2018

Hey @MicheleBertoli,
quick PR to fix a small edge case introduced in #206.

This is to fix a scenario which would cause the following test to fail when it shouldn't

  const Button = styled.button`
    opacity: .65;
  `

  expect(<Button />).not.toHaveStyleRule('opacity', undefined)

Info

Since the opacity rule is applied to Button we can also write a test to check that the rule should not be undefined.
I consider this an edge case since this test is certainly convoluted and possibly pointless as most likely if you have a rule applied you want to write a test to check its value.

Fix

When the requested rule is undefined and the expected value is not passed or falsy and the negated .not modifier is used then we can return directly false to make our test pass without any need to use the matcherTest util which would always return true, since it evaluates expect(undefined).toEqual(undefined).
If the above conditions don't apply then we leave the matching logic to the matcherTest util as before.

This approach avoid the invocation of a function when is not required (as the result is predictable) and also simplifies the logic as it doesn't require flipping of the assertion.

Notes

I actually realised this was probably going to create the issue mentioned above the day after opening #206 but have had a very crazy week and didn't manage to push a fix earlier.
Now that is weekend I finally got 5mins to get this sorted; sorry it will require you to publish v6.3.1 馃檹

@MicheleBertoli
Copy link
Member

Thank you very much, @santino.
I would also consider .not undefined an edge case, but I appreciate you enabled the ability to use it.
As usual, awesome PR 馃檶

@MicheleBertoli MicheleBertoli merged commit 7103cea into styled-components:master Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants