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

Add toHaveStyleRule to jest-emotion #662

Merged
merged 9 commits into from
May 25, 2018

Conversation

danreeves
Copy link
Contributor

@danreeves danreeves commented May 16, 2018

What:

Feature: adding toHaveStyleRule to jest-emotion.

Why:

This utility is provided by jest-styled-components and jest-glamor-react, so is a blocker for people wanting to port to emotion. Already tracked here: #645

How:

I originally wrote the utility for jest-glamor-react and that was in turn based on the same utility in jest-styled-components.

Checklist:

  • Documentation
  • Tests
  • Code complete

// This could be done in a more efficient way
// but it would be a breaking change to do so
// because it would change the ordering of styles
Object.keys(emotion.caches.registered).forEach(className => {
Copy link
Member

Choose a reason for hiding this comment

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

personally I would replace forEach with reduce here

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 actually copied that block of code from index.js because it did what I needed. Agree it should be refactored 👍

@@ -37,12 +39,14 @@ function getClassNamesFromDOMElement(selectors, node) {
return getClassNames(selectors, node.getAttribute('class'))
}

function getClassNamesFromNodes(nodes) {
export function getClassNamesFromNodes(nodes) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this into a utils file and import it from that file because exporting from here means it’s exported from the main package so people could start using it and if we wanted to change it we would have to do a major version

@codecov
Copy link

codecov bot commented May 18, 2018

Codecov Report

Merging #662 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/jest-emotion/src/index.js 80.64% <ø> (-3.98%) ⬇️
packages/jest-emotion/src/utils.js 100% <100%> (ø)
packages/jest-emotion/src/matchers.js 100% <100%> (ø)

@danreeves danreeves changed the title [WIP] toHaveStyleRule Add toHaveStyleRule to jest-emotion May 18, 2018
@danreeves
Copy link
Contributor Author

It should be code complete now, I'll see what I can do about increasing the code coverage though.

I'm not sure how to add a new dependency to a package. I tried ./node_modules/.bin/lerna add --scope=jest-emotion chalk but it didn't do anything.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

@emmatown emmatown merged commit 0210b94 into emotion-js:master May 25, 2018
@danreeves
Copy link
Contributor Author

Thanks!

@daddyj
Copy link

daddyj commented Jun 5, 2018

Hey guys! Great that this feature is finally merged to master, really looking forward to using it!
Any timetable for the next release including this feature?

@Andarist
Copy link
Member

Andarist commented Jun 5, 2018

We hope to make a new release this week, can't promise anything though.

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.

5 participants