Migrate "theming" library #292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #292 +/- ##
==========================================
+ Coverage 97.36% 97.73% +0.37%
==========================================
Files 17 22 +5
Lines 569 662 +93
Branches 134 152 +18
==========================================
+ Hits 554 647 +93
Misses 11 11
Partials 4 4
Continue to review full report at Codecov.
|
tkh44
added
breaking change
Tag: Enhancement
and removed
breaking change
Tag: Documentation
labels
|
I'll finish this up tonight |
| "clean": "rimraf dist", | ||
| "build": "run-s clean babel:*", | ||
| "babel:cjs": "cross-env BABEL_ENV=cjs babel src -d dist/cjs", | ||
| "babel:esm": "cross-env BABEL_ENV=esm babel src -d dist/esm" |
There was a problem hiding this comment.
Could you change this to use the rollup setup we have for the other packages?
https://github.com/emotion-js/emotion/blob/master/packages/preact-emotion/package.json#L12-L14
| "react-dom": "^15.5.4", | ||
| "react-test-renderer": "^15.5.4", | ||
| "rimraf": "^2.6.1" | ||
| }, |
There was a problem hiding this comment.
Could you remove all the devDependencies except rollup, rimraf, npm-run-all and I think prop-types since it's not in the root package.json. It's better to have devDependencies in the root package.json because otherwise they have to be installed multiple times except for packages where we use binaries since binaries aren't linked.
package.json
Outdated
| @@ -8,7 +8,7 @@ | |||
| "clean": "lerna run clean --parallel", | |||
| "test": "npm-run-all -p lint:check coverage test:size", | |||
| "coverage": "jest --coverage --no-cache --ci --runInBand", | |||
| "lint:check": "eslint .", | |||
| "lint:check": "eslint . --fix", | |||
There was a problem hiding this comment.
Please change this back. This is for CI to check that there are no linting errors.
There was a problem hiding this comment.
I will, debugging something else
|
Ok so I came up with an interesting bug while trying to finish this... looking for feedback. Basically if you have a situation where you're using Something like this: const theme = { color: 'red' }
const Component = styled.div`
color: ${p => p.theme.color};
`
const ThemedComponent = withTheme(Component)
const FinalComponent = styled(ThemedComponent)`
border: 1px solid blue;
`
const wrapper = mount(
<ThemeProvider theme={theme}>
<FinalComponent />
</ThemeProvider>
)However, this only happens with static hoisting... if I remove the hoisting the error goes away, but then component selector will no longer work. |
|
We could get away with doing this: hoist(WithTheme, Component)
delete WithTheme.__emotion_specany objections? update: pushed this fix in 9478fb4 |
| @@ -23,6 +23,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "babel-cli": "^6.24.1", | |||
| "emotion-theming": "^1.0.0", | |||
There was a problem hiding this comment.
This doesn't need to be a devDependency here since it's in jest's moduleNameMapper.
There was a problem hiding this comment.
I’m on vacation until Tuesday, so if you want to get this in sooner feel free to push that change on.
Had to add a babel plugin to silence a rollup warning.
|
@tkh44 @mitchellhamilton all comments should be addressed |
…otion into es-migrate-theming
|
Wait why is component selector being removed? It’s quite hard to achieve the same functionality without first class support, and the majority of the reason I even opened this PR :/ |
|
Thank you for putting all this work in @probablyup 🍻 |
Fixes: #274
Checklist:
The only thing left I think is to update the existing docs to point toemotion-theminginstead.