-
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
[ML] Migrate influencers list from SCSS to Emotion #200019
[ML] Migrate influencers list from SCSS to Emotion #200019
Conversation
Pinging @elastic/ml-ui (:ml) |
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.
Approving this despite the number of curious, hardcoded pixel values in x-pack/plugins/ml/public/application/components/influencers_list/influencers_list_styles.ts
I presume the aim here is to address the removal over SCSS and not rework the styles.
import { mlColors } from '../../styles'; | ||
import { useMlKibana } from '../../contexts/kibana'; | ||
|
||
export const useInfluencersListStyles = () => { |
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.
As shown in your before / after screenshots, the total height of each influencer div
has decreased slightly (looks like from 41 to 36px in my testing). On the plus side, this means less vertical whitespace. But on the downside, the progress bar now looks closer to the influencer below it than it's own text label.
Do you know what is causing the change in total height? I couldn't track down the source of the change.
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.
I played a little bit more with the layout, but still can't find the cause.
One could be that top margin for the total score number is removed, though it's only about 2px.
It is kinda hard to catch, as initially, I couldn't get the layout correct. After some adjustments, I discovered that the progress bar had float: left
applied from another file 🤔 . I aligned it more closely with the SCSS styles, but the height remains smaller. We could add some margin to separate it more from the next influencer.
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.
I'm happy to go with what you have now. The slightly compressed height is a win.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
cc @rbrtj |
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.
LGTM
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.
LGTM ⚡
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11922863147 |
## Summary Part of: [elastic#140695](elastic#140695) Migrates SCSS to emotion for Influencers list. | Before | After | | ------------- | ------------- | | ![image](https://github.com/user-attachments/assets/1f85b2d1-5526-49b2-819d-525989b9c48d) | ![image](https://github.com/user-attachments/assets/e1e5745e-d00a-4a51-ab93-b29ff71d8aef) | | ![image](https://github.com/user-attachments/assets/f1e8d594-8a9d-4f08-98bb-156e21abd1c6) | ![image](https://github.com/user-attachments/assets/d55a2848-2c28-4b4b-88b2-ed1b98b16430) | (cherry picked from commit 9162fde)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…200824) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Migrate influencers list from SCSS to Emotion (#200019)](#200019) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Robert Jaszczurek","email":"92210485+rbrtj@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-11-19T22:23:41Z","message":"[ML] Migrate influencers list from SCSS to Emotion (#200019)\n\n## Summary\r\n\r\nPart of: [#140695](https://github.com/elastic/kibana/issues/140695)\r\nMigrates SCSS to emotion for Influencers list.\r\n\r\n| Before | After |\r\n| ------------- | ------------- |\r\n|\r\n![image](https://github.com/user-attachments/assets/1f85b2d1-5526-49b2-819d-525989b9c48d)\r\n|\r\n![image](https://github.com/user-attachments/assets/e1e5745e-d00a-4a51-ab93-b29ff71d8aef)\r\n|\r\n|\r\n![image](https://github.com/user-attachments/assets/f1e8d594-8a9d-4f08-98bb-156e21abd1c6)\r\n|\r\n![image](https://github.com/user-attachments/assets/d55a2848-2c28-4b4b-88b2-ed1b98b16430)\r\n|","sha":"9162fdea146adadec05fa50f2cf4461bf19b7892","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","v9.0.0","Team:ML","backport:version","v8.17.0"],"title":"[ML] Migrate influencers list from SCSS to Emotion","number":200019,"url":"https://github.com/elastic/kibana/pull/200019","mergeCommit":{"message":"[ML] Migrate influencers list from SCSS to Emotion (#200019)\n\n## Summary\r\n\r\nPart of: [#140695](https://github.com/elastic/kibana/issues/140695)\r\nMigrates SCSS to emotion for Influencers list.\r\n\r\n| Before | After |\r\n| ------------- | ------------- |\r\n|\r\n![image](https://github.com/user-attachments/assets/1f85b2d1-5526-49b2-819d-525989b9c48d)\r\n|\r\n![image](https://github.com/user-attachments/assets/e1e5745e-d00a-4a51-ab93-b29ff71d8aef)\r\n|\r\n|\r\n![image](https://github.com/user-attachments/assets/f1e8d594-8a9d-4f08-98bb-156e21abd1c6)\r\n|\r\n![image](https://github.com/user-attachments/assets/d55a2848-2c28-4b4b-88b2-ed1b98b16430)\r\n|","sha":"9162fdea146adadec05fa50f2cf4461bf19b7892"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200019","number":200019,"mergeCommit":{"message":"[ML] Migrate influencers list from SCSS to Emotion (#200019)\n\n## Summary\r\n\r\nPart of: [#140695](https://github.com/elastic/kibana/issues/140695)\r\nMigrates SCSS to emotion for Influencers list.\r\n\r\n| Before | After |\r\n| ------------- | ------------- |\r\n|\r\n![image](https://github.com/user-attachments/assets/1f85b2d1-5526-49b2-819d-525989b9c48d)\r\n|\r\n![image](https://github.com/user-attachments/assets/e1e5745e-d00a-4a51-ab93-b29ff71d8aef)\r\n|\r\n|\r\n![image](https://github.com/user-attachments/assets/f1e8d594-8a9d-4f08-98bb-156e21abd1c6)\r\n|\r\n![image](https://github.com/user-attachments/assets/d55a2848-2c28-4b4b-88b2-ed1b98b16430)\r\n|","sha":"9162fdea146adadec05fa50f2cf4461bf19b7892"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Robert Jaszczurek <92210485+rbrtj@users.noreply.github.com>
## Summary Part of: [elastic#140695](elastic#140695) Migrates SCSS to emotion for Influencers list. | Before | After | | ------------- | ------------- | | ![image](https://github.com/user-attachments/assets/1f85b2d1-5526-49b2-819d-525989b9c48d) | ![image](https://github.com/user-attachments/assets/e1e5745e-d00a-4a51-ab93-b29ff71d8aef) | | ![image](https://github.com/user-attachments/assets/f1e8d594-8a9d-4f08-98bb-156e21abd1c6) | ![image](https://github.com/user-attachments/assets/d55a2848-2c28-4b4b-88b2-ed1b98b16430) |
## Summary Part of: [elastic#140695](elastic#140695) Migrates SCSS to emotion for Influencers list. | Before | After | | ------------- | ------------- | | ![image](https://github.com/user-attachments/assets/1f85b2d1-5526-49b2-819d-525989b9c48d) | ![image](https://github.com/user-attachments/assets/e1e5745e-d00a-4a51-ab93-b29ff71d8aef) | | ![image](https://github.com/user-attachments/assets/f1e8d594-8a9d-4f08-98bb-156e21abd1c6) | ![image](https://github.com/user-attachments/assets/d55a2848-2c28-4b4b-88b2-ed1b98b16430) |
Summary
Part of: #140695
Migrates SCSS to emotion for Influencers list.