-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Enable CSS-in-JS styling with emotion
#98157
Changes from 16 commits
778627a
196bf60
b60a64e
d596827
db03924
fd81944
014f2f0
f457a73
aceaed2
255a812
df51fff
d815ad9
59ac889
c11ebb5
361e2f1
3bfddfd
c877306
cd45fda
6b3d432
b008566
f811056
985577b
1dda574
c777224
3972208
8bdfd6b
cc91272
f6a1e7e
47455f1
4a93b5b
b5d2633
a99dc6a
145a606
7547d66
cbf4c94
b464828
447e80e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ TYPES_DEPS = [ | |
"@npm//@types/node", | ||
"@npm//@types/node-forge", | ||
"@npm//@types/testing-library__jest-dom", | ||
"@npm//@emotion/react", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mistic does this make sense to you? Or do we think it's one of dependency ordering issues? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was some related discussion in #99382 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it makes sense as we are inheriting from the |
||
] | ||
|
||
DEPS = SRC_DEPS + TYPES_DEPS | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,8 @@ | |
"types": [ | ||
"node", | ||
"jest", | ||
"react" | ||
"react", | ||
"@emotion/react/types/css-prop" | ||
] | ||
}, | ||
"include": [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,10 @@ | |
// Allows for importing from `kibana` package for the exported types. | ||
"kibana": ["./kibana"], | ||
"kibana/public": ["src/core/public"], | ||
"kibana/server": ["src/core/server"] | ||
"kibana/server": ["src/core/server"], | ||
"@emotion/core": [ | ||
"typings/@emotion" | ||
], | ||
Comment on lines
+16
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stubs the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe Storybook should use a dedicated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EUI PRs like this have highlighted the need to make better use of the various There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If @elastic/kibana-operations team thinks it's acceptable... @mistic any actionable items for the problem? |
||
}, | ||
// Support .tsx files and transform JSX into calls to React.createElement | ||
"jsx": "react", | ||
|
@@ -54,7 +57,8 @@ | |
"jest", | ||
"flot", | ||
"jest-styled-components", | ||
"@testing-library/jest-dom" | ||
"@testing-library/jest-dom", | ||
"@emotion/react/types/css-prop" | ||
] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
// Stub @emotion/core | ||
// Remove when @storybook has moved to @emotion v11 | ||
thompsongl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a useful error message if a developer tries to use styled-components outside these directories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently, but if the ops folks agree that this opt-in/opt-out approach is the right way to go, we can look at adding a message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to discouraging use of styled-components and push the teams working on these plugins to migrate to emotion?
What if we added an eslint error message for importing styled-components that told people to use
@emotion/react
instead. Then people would need to disable the eslint rule when importing it which I think is ideal if we're actively discouraging the spread of styled-compononents. We could also have the opposite rule in place and maintain this list of selectors in.eslintrc.js
or something so they stay synchronized.(This is quite a list of plugins already using styled-components though, didn't expect it to be so large)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The APIs are similar enough that migration shouldn't be time consuming. I was also surprised to see so many plugins using it; the list has grown since I last did an audit.
Good ideas regarding eslint. I'll look in to creating a rule and some kind of synchronizable list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I think since we're going to need to scope this to specific files we might want to customize the
@kbn/eslint/module_migration
rule: https://github.com/elastic/kibana/blob/master/packages/elastic-eslint-config-kibana/.eslintrc.js#L37-L41We can add file globs to include/exclude specific migration mappings for specific patterns using regex like above. Should be as simple as filtering the list of mappings and then returning an empty visitor
{}
when the mappings are empty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a meta issue with an explicit deadline to track the migration process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got back to this. Extended
@kbn/eslint/module_migration
to acceptinclude
andexclude
and used that to write a new rule forstyled-components
. The file regex array used by@kbn/babel-preset
is now stored in@kbn/dev-utils
and is shared across packages.