-
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
[Observability plugin] Audit new EUI Borealis theme #204615
Conversation
Don't you need to change this file: packages/kbn-babel-preset/styled_components_files.js as well in order to not use styled component anymore? |
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.
Code change LGTM. Just a question regarding the need to update: packages/kbn-babel-preset/styled_components_files.js
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
✅ Changes LGTM from EUI side
@kdelemme I don't know. This is the first time I have checked this file. Can you elaborate more on what needs to be updated in this file and what would be the impact? It looks like it is related to bundling. |
@fkanout it looks like this line https://github.com/elastic/kibana/blob/main/packages/kbn-babel-preset/styled_components_files.js#L18 mentions files that we have presumably just edited to no longer use styled components. The comments of that file mention:
So I think we are probably helping those tools so they can stop trying to lint or build those files with styled components in mind. I suspect leaving it there is a no-op but we should probably remove. Can you confirm that all of the listed regex patterns in that line have been updated in this PR and if so, remove the line? Then we can get this merged. Thanks for working on this. |
@jasonrhodes line 18 in the link is pointing to
Not all of these files have been migrated from Styled-components. Here is the list of the remaining ones:
I will remove the regex for the other plugins/pages. |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
History
cc @fkanout |
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.
Changes to the styled_components_files.js file LGTM -- I spoke with Faisal about why streams is removed here and it appears they've done some PRs to remove style components there recently too, so we're removing them from this file for them. (I believe those lines were recently added here just yesterday or today, in an attempt to decrease the conflicts in this file because of a wider regex that was in use in one line.)
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.
entity_manager change lgtm
## Summary It fixes elastic#203338 by updating tokens and migrating from `styled-components` to `@emotion` and Eui Visual Refresh for Borealis theme for the files owned by @elastic/obs-ux-management-team.
## Summary It fixes elastic#203338 by updating tokens and migrating from `styled-components` to `@emotion` and Eui Visual Refresh for Borealis theme for the files owned by @elastic/obs-ux-management-team.
Summary
It fixes #203338 by updating tokens and migrating from
styled-components
to@emotion
and Eui Visual Refresh for Borealis theme for the files owned by @elastic/obs-ux-management-team.