[Discover] SCSS to Emotions migration part 2/4#214729
[Discover] SCSS to Emotions migration part 2/4#214729akowalska622 merged 17 commits intoelastic:mainfrom
Conversation
There was a problem hiding this comment.
Those classes were not used anywhere, removed file and confirmed component remains styled the same
|
/ci |
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
davismcphee
left a comment
There was a problem hiding this comment.
Code changes look good and work well overall! I did notice a couple of issues to look into and left some feedback.
...atform/plugins/shared/discover/public/application/main/components/layout/discover_layout.tsx
Show resolved
Hide resolved
...atform/plugins/shared/discover/public/application/main/components/layout/discover_layout.tsx
Outdated
Show resolved
Hide resolved
...cation/main/components/no_results/no_results_suggestions/assets/no_results_illustration.scss
Show resolved
Hide resolved
...shared/discover/public/application/main/components/skip_bottom_button/skip_bottom_button.tsx
Outdated
Show resolved
Hide resolved
...tform/plugins/shared/discover/public/application/main/components/top_nav/discover_topnav.tsx
Outdated
Show resolved
Hide resolved
davismcphee
left a comment
There was a problem hiding this comment.
A drop in the async bundle, 100 more lines removed than added, and lots of tech debt / dead code removal. What's not to love about this PR? Confirmed both issues I noticed are resolved, and in that case it LGTM, thanks!
...atform/plugins/shared/discover/public/application/main/components/layout/discover_layout.tsx
Show resolved
Hide resolved
...atform/plugins/shared/discover/public/application/main/components/layout/discover_layout.tsx
Outdated
Show resolved
Hide resolved
...cation/main/components/no_results/no_results_suggestions/assets/no_results_illustration.scss
Show resolved
Hide resolved
...shared/discover/public/application/main/components/skip_bottom_button/skip_bottom_button.tsx
Outdated
Show resolved
Hide resolved
...tform/plugins/shared/discover/public/application/main/components/top_nav/discover_topnav.tsx
Outdated
Show resolved
Hide resolved
Sounds like a book review! 😄 I'm happy to contribute 🚀 |
l-suarez
left a comment
There was a problem hiding this comment.
Thanks for the ping Ania, LGTM
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
History
|
Summary
SCSS to Emotions migration - part 2/4
Part of #209807
Approach
All CSS-in-JS styles, which can be extracted outside of the component without issues are extracted below the component and euiTheme is passed as an argument
If it's not possible, because of state changes (which impacts styles) or unit tests issues, those styles are defined inline and usually use
useEuiThemehookClass are removed when it can be done without any issues. If a class is used in tests or different style block uses it as a child class for specificity reasons, I left the class assigned to the component. All places like this are described with a proper comment.
If some props-to-class could be easily migrated to conditional CSS attribute value it was migrated. Some of them though were used in cases described in point 3) and/or were too convoluted to easily and clearly migrate it to conditional values (for example
src/platform/packages/shared/kbn-unified-field-list/src/components/field_item_button/field_item_button.tsx, but there are more).Approach was inspired by [FAQ] Kibana and Emotion / CSS-in-JS usage. If we opt for a different approach, I'm open for suggestions and I'll align it accordingly.
Files included
src/platform/plugins/shared/discover/public/application/context/components/action_bar/_action_bar.scsssrc/platform/plugins/shared/discover/public/application/context/context_app.scsssrc/platform/plugins/shared/discover/public/application/main/components/layout/discover_layout.scsssrc/platform/plugins/shared/discover/public/application/main/components/no_results/_no_results.scsssrc/platform/plugins/shared/discover/public/application/main/components/no_results/no_results_suggestions/assets/no_results_illustration.scsssrc/platform/plugins/shared/discover/public/application/main/components/skip_bottom_button/skip_bottom_button.scsssrc/platform/plugins/shared/discover/public/application/main/components/top_nav/top_nav.scsssrc/platform/plugins/shared/discover/public/embeddable/components/saved_search_grid.scssChecklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesIdentify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.