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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not lower-case css values for comparison in toHaveValues #163

Closed
wants to merge 1 commit into from

Conversation

gnapse
Copy link
Member

@gnapse gnapse commented Nov 14, 2019

What:

Change toHaveValues to not lowercase css values.

Why:

See full discussion in this comment in the PR that introduced this behavior.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #163 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #163   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          19     19           
  Lines         234    234           
  Branches       57     57           
=====================================
  Hits          234    234
Impacted Files Coverage Δ
src/to-have-style.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c12a476...a78c0ff. Read the comment docs.

@connormeredith
Copy link
Contributor

🙌 Thanks for addressing this @gnapse. Can you add a regression test to show we can match transform: translateX(0px);?

@gnapse
Copy link
Member Author

gnapse commented Nov 14, 2019

Can you add a regression test to show we can match transform: translateX(0px);?

Yes, that's in the plans. I haven't because I quickly drafted this in the browser using GitHub's UI.

@connormeredith
Copy link
Contributor

Awesome thanks 😄 I had a branch ready to go but you beat me to the PR, but something like this will probably suffice: master...connormeredith:toHaveStyle-fix

@gnapse
Copy link
Member Author

gnapse commented Nov 14, 2019

@connormeredith TBH I wouldn't mind if you put that up as a PR that would supersede this one. I'm on the go and can help with code review, approvals, merging stuff, but will not get to the rest of this PR until later today. We better get this ASAP. Let me know and I can close this one in favor of yours.

@connormeredith
Copy link
Contributor

@gnapse sounds good to me: #164

@gnapse
Copy link
Member Author

gnapse commented Nov 14, 2019

Superseded by #164

@gnapse gnapse closed this Nov 14, 2019
@nickmccurdy nickmccurdy deleted the gnapse-patch-1 branch November 11, 2020 22:00
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