Skip to content

feature: toHaveStyle custom matcher #12

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

Merged
merged 7 commits into from
May 17, 2018
Merged

feature: toHaveStyle custom matcher #12

merged 7 commits into from
May 17, 2018

Conversation

gnapse
Copy link
Member

@gnapse gnapse commented May 17, 2018

What:

A new .toHaveStyle custom matcher, as proposed in #6.

Closes #6

Checklist:

  • Documentation
  • Tests
  • Replace parseCSS with a more robust alternative (like this)
  • Support passing styles in POJO form (?)

@gnapse gnapse added the enhancement New feature or request label May 17, 2018
import {matcherHint} from 'jest-matcher-utils'
import {checkHtmlElement, getMessage} from './utils'

function parseCSS(css) {
Copy link
Member

Choose a reason for hiding this comment

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

You might consider using the css module for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will. It's on the to-do list above. This was just a first draft, a proof-of-concept.

@codecov-io
Copy link

codecov-io commented May 17, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      7    +1     
  Lines          41     67   +26     
  Branches       12     15    +3     
=====================================
+ Hits           41     67   +26
Impacted Files Coverage Δ
src/utils.js 100% <ø> (ø) ⬆️
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 bbdd96c...60ae40d. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is going to be soooo cool!

In the future maybe we could consider toHaveOnlyStyles, or maybe we could call this method toHaveStylesSubset or something.

)
}

function printoutStyles(styles) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use css.stringify to ensure that this handles things like media queries and stuff properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but there's a problem with this. The css library seems to only be able to handle the syntax of css to the stylesheet level, that is, property: value; pairs listed inside a selector { ... }. And here I'm handling only plain css property/value pairs (well, sets of property/value pairs really) abstracted away from a selector. I do not even think media queries apply.

For instance, I'd have to convert what getComputedStyles give me, which is, again, a set of plain css property/value pairs, and manually build the AST that the css lib would have created out of those rules with a fake css selector wrapping them, just to make css.stringify be able to process it.

I saw the point of using css to parse because even though I had to fake wrapping the plain css props in a selector, it gives me syntax error handling, which is amazing for the pretty error messages when expectations in tests fail. But after all this reasoning do you still think is worth using css.stringify @kentcdodds?

Copy link
Member Author

Choose a reason for hiding this comment

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

(Do we even need to handle media queries here? Maybe I'm missing out some potential great use case for this, but I don't see it yet)

Copy link
Member

Choose a reason for hiding this comment

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

You're right, I don't think we need to use css.stringify 👍

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I think this is amazing! Thanks for working on this!

It would be very cool in the future to support objects in addition to a string of CSS 👍

@kentcdodds
Copy link
Member

kentcdodds commented May 17, 2018

I'll let you go ahead and merge this considering this is your project and all. 👍

I'm just a bystander 😉

@gnapse
Copy link
Member Author

gnapse commented May 17, 2018

It would be very cool in the future to support objects in addition to a string of CSS

I considered working on that in this PR, but I'll leave it for now, so we can have something usable sooner.

I'll let you go ahead and merge this considering this is your project and all

I think I never thanked you for this huge opportunity to work on and maintain an OSS project, almost all by myself, so here it goes: Thank you! (though I still don't think of it as mine, never would have worked on this if it weren't for your and @antoaravinth who led the way, thanks again!)

@gnapse gnapse merged commit 6b575db into master May 17, 2018
@gnapse gnapse deleted the pr/to-have-style branch May 17, 2018 22:05
@gnapse
Copy link
Member Author

gnapse commented May 17, 2018

Ooops, I think I messed up @kentcdodds. I think the commit was supposed to start with feat:, not feature: 😬 and now I think it won't trigger a release.

What am I supposed to do now? Something like what you did here?

You see why I still need you around? 😄

@kentcdodds
Copy link
Member

Haha, no worries. Yep, that's exactly what the manual_releases.md file is for 👍

gnapse added a commit that referenced this pull request May 17, 2018
There was an issue with a minor release, so this manual-releases.md
change is to release a new minor version.

Reference: #12
@gnapse
Copy link
Member Author

gnapse commented May 17, 2018

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sigfriedCub1990
Copy link

This is going to be soooo cool!

In the future maybe we could consider toHaveOnlyStyles, or maybe we could call this method toHaveStylesSubset or something.

Is this feature implemented? If it isn't I would love to take a look at @gnapse job and try to figure out what can be done in order to solve the problem. By the way, huge thanks to you @kentcdodds for inspiring other people to contribute and learn doing OSS :)

@gnapse
Copy link
Member Author

gnapse commented Jan 22, 2020

In the future maybe we could consider toHaveOnlyStyles, or maybe we could call this method toHaveStylesSubset or something.

Not sure what's the logic there. The fact that it asserts that the element has some styles does not imply that those have to be all the styles it's got. It's only that those are the style rules you want to assert. The same happens with .toHaveClass. It asserts that the element has a given css class, regardless of it possibly having other classes irrelevant to the test.

Unless I'm misunderstanding?

@sigfriedCub1990
Copy link

sigfriedCub1990 commented Jan 22, 2020

Hi @gnapse 👋,

Thanks for the response, if that's the case, yes, I think that the statement is a bit redundant. Actually I realized that the thing I was trying to test wasn't working as I expected, long story short: I was having issues with the fireEvent.focus, thankfully the docs are really good and I also found a comment you did in another issue.
So, part misunderstanding by my side both for how to test and library usage.

Have a nice day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New toHaveStyle custom matcher
4 participants