Convert all Less files to Sass in discover, use EUI variable scope #21290
Conversation
| @@ -0,0 +1,6 @@ | |||
| // EUI global scope | |||
| @import '../../../../node_modules/@elastic/eui/src/themes/k6/k6_globals'; | |||
| @import '../../../../node_modules/@elastic/eui/src/themes/k6/k6_colors_light'; | |||
There was a problem hiding this comment.
How does/will dark theme work if you are importing just the light colors?
There was a problem hiding this comment.
@trevan The short term plan is:
During 6.5 dev we're converting all the Less files to Sass. We will maintain current "theming" capability (dark mode for dashboards). In the Dashboard's sass files, this will have a similar .theme-dark selector hack where the sass files will include k6_colors_dark and overwrite these top level imports.
Beyond that work we'd like to get full theming available through the switch of actual compiled css files (negating the need for the .theme-dark selector approach). This will open up all of Kibana to theming similar to what you can see on https://elastic.github.io/eui/#/
End goal is that Kibana at large should be theme-able by changing a few lines of Sass, which is why we're doing all this work now.
💔 Build Failed |
There was a problem hiding this comment.
Good job. I'll need some time for a deeper scan in the browser but I didn't see anything specifically off.
I think you could probably do a pass where you used $eui sizing vars for a lot of the pixel values here. It would require you to pay more attention to the display in the browser (since some of these things line up).
If you're not comfortable, that's OK, either @cchaos or I can do it in a later pass.
You should feel safe deleting selectors if you don't see them used in the templates, which were the majority of my comments.
| } | ||
|
|
||
| // SASSTODO: Seems that this selector is not used anywhere | ||
| visualize { |
There was a problem hiding this comment.
This looks safe to delete. It looks like a style tag applies the height and the "spy" is removed.
| } | ||
|
|
||
| // SASSTODO: Seems that this class is not used anywhere | ||
| .chartwrapper { |
| } | ||
|
|
||
| // SASSTODO: Seems that this class is not used anywhere | ||
| .vis-overlay { |
| } | ||
|
|
||
| // SASSTODO: seems like this class is not used anywhere | ||
| .discover-info { |
| } | ||
|
|
||
| // SASSTODO: seems like this class is not used anywhere | ||
| .discover-info-title { |
| margin-bottom: 3px; | ||
| } | ||
|
|
||
| .dscTable__timefield { |
There was a problem hiding this comment.
I'd remove this and replace it with .eui-textNoWrap in the template.
https://elastic.github.io/eui/#/utilities/css-utility-classes
|
|
||
| // SASSTODO: seems like this class is not being used anywhere | ||
| .discover-field-details-count { | ||
| white-space: nowrap; |
There was a problem hiding this comment.
I'd remove this and replace it with .eui-textNoWrap in the template.
https://elastic.github.io/eui/#/utilities/css-utility-classes
| padding-bottom: 0 !important; /* 1 */ | ||
| height: $euiSizeXL; | ||
|
|
||
| &--active { |
There was a problem hiding this comment.
I think we should probably write them out fully to make search/replace easier. @cchaos ?
There was a problem hiding this comment.
You're right, besides we use full selector names everywhere. Fixed this one
| } | ||
|
|
||
| // SASSTODO: Seems that .discover-field-details is not used anywhere | ||
| .discover-field-details { |
| .discoverNoResultsShardFailure { | ||
| display: block; | ||
|
|
||
| .euiFlexItem:last-child { |
There was a problem hiding this comment.
Apply a new selector to the template on top. We never want to overwrite .eui classes in Kibana, even if its a deep selection like this.
<EuiFlexItem className="myThing">
|
I can help get the tests to pass once we're cleaned up. You'll also want to rebase or merge master against this branch to remove the conflict. |
going-confetti
force-pushed
the
sass/discover
branch
3 times, most recently
from
23f2031
to
682452d
Compare
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
going-confetti
force-pushed
the
sass/discover
branch
from
682452d
to
cbd561b
Compare
💔 Build Failed |
going-confetti
force-pushed
the
sass/discover
branch
from
cbd561b
to
90c8525
Compare
💔 Build Failed |
going-confetti
force-pushed
the
sass/discover
branch
from
90c8525
to
0288f8f
Compare
💚 Build Succeeded |
|
@Bargs @lukasolson We have a PR going that converts all of discovery's LESS files into SASS. There should be no visual changes, this is just converting the code and using EUI variables. Do either of you have time to do a review? Specifically just need you to look through discover in the browser and check for any breaks. |
|
I might be able to do a visual check later this week. |
going-confetti
force-pushed
the
sass/discover
branch
from
0288f8f
to
56bbe72
Compare
💔 Build Failed |
|
@Bargs Thanks, I just fixed the issue. |
|
jenkins, retest this |
💔 Build Failed |
|
jenkins test this. |
💚 Build Succeeded |
There was a problem hiding this comment.
Thanks @Bargs @lukasolson. We'll merge this as soon as we're unblocked by #21365.
💚 Build Succeeded |
|
jenkins, test this |
💚 Build Succeeded |
going-confetti
force-pushed
the
sass/discover
branch
from
538c02c
to
72ad6a3
Compare
💔 Build Failed |
|
jenkins, retest this |
💚 Build Succeeded |
…lastic#21290) Discover now uses sass for its styling
…lastic#21290) Discover now uses sass for its styling
…lastic#21290) Discover now uses sass for its styling
This PR removes the LESS files for the discover plugin and replaces them with Sass. It follows the same process taken in #20995
It is dependent on #21365