Machine learning convert Less to Scss#23387
Conversation
0ed7362 to
22d7b10
Compare
💚 Build Succeeded |
22d7b10 to
3f75189
Compare
|
@peteharverson @walterra @jgowdyelastic I'm sure there are plenty of breaks in here still, but at this point I could prolly use a visual review so I can start fixing things. Takes screens of whatever you notice and I'll do another pass. Note that I'm really trying NOT to touch layouts or designs too much. This is really just a base run to get the code workable so it has some level of variable scoping. Colors will be be slightly different in places, and in many cases this fixes a lot of color contrast accessibility issues in ML, but if something looks really off, it's likely a mistake on my part. I don't think much of the chart coloring should change (other than the grays) and I've introduced a small ML specific variable layer into the sass so things are at least consistent till my team can do more work in here. |
💚 Build Succeeded |
|
For the most part this is looking great @snide! I have taken a first quick(ish) run through all the pages, and the only areas which have significant breaks are the Single and Multi-metric job wizards. Some issues on other pages, but those are all fairly minor I think. I have saved a bunch of screenshots from my review - will be in contact to point you at the doc. |
|
Pinging @elastic/ml-ui |
a3dab88 to
e848718
Compare
|
OK @peteharverson. Think I got most everything from your list. Have some small issues around the bootstrap autocomplete placeholders, but I think we're close to what master looks like now. |
💔 Build Failed |
|
Tested your latest fixes @snide. Getting very close to master I think. I am still seeing the issue with the layout of the top card in the Population Job wizard. I have also added one extra item to the end of the doc related to the |
💔 Build Failed |
d166a7a to
27a4805
Compare
💚 Build Succeeded |
|
@cchaos I know this is a big blob to review so I don't expect a line by line review. The ML team covered most of the visual stuff so I'm pretty sure this looks pretty similar to how it used to. Can you do a quick scan of at least the sandboxing, sass importing and general structure of the code? Since ML is an app in transition, you'll often find folders with titles like |
x-pack/plugins/ml/public/_app.scss
Outdated
There was a problem hiding this comment.
Just a note for review. I avoided touching shadows on this pass. A lot of ML uses text shadows and drops that I didn't have a quick 1 to 1 parallel for. Mostly I'd change the color to match against the EUI counterpart and then move forward. Rest can be handled on a 2nd pass.
There was a problem hiding this comment.
This particular one looks like the EUI counterpart is significantly lighter. The original being #BBBBBB and this one equates to #F5F5F5, but I'm also not looking at it visually.
💔 Build Failed |
💚 Build Succeeded |
|
@snide I tested out the latest edits, and the layout of the top card in the Population wizard looks good. I have added a couple of extra items to the bottom of the doc - one for padding around the population wizard charts, and one for a suggestion to switch the border color used in the population wizards to |
x-pack/plugins/ml/public/_app.scss
Outdated
There was a problem hiding this comment.
This particular one looks like the EUI counterpart is significantly lighter. The original being #BBBBBB and this one equates to #F5F5F5, but I'm also not looking at it visually.
There was a problem hiding this comment.
Do you think this is a bit bright on top of the dark tooltip background? Original color was: #95a5a6
There was a problem hiding this comment.
Maybe move these up under the previous selector .dropdown-menu > li > a using &:hover, etc...
x-pack/plugins/ml/public/components/documentation_help_link/_documentation_help_link.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Supposed to be negative, original: -10px
x-pack/plugins/ml/public/index.scss
Outdated
x-pack/plugins/ml/public/index.scss
Outdated
|
OK. I think I got to all of the review feedback. THANK YOU @cchaos for the detail on such a mighty PR. 🥇 |
💔 Build Failed |
💔 Build Failed |
|
jenkins, test this |
💔 Build Failed |
9a6cdb8 to
b30be74
Compare
💔 Build Failed |
|
Replaced with #25574 |

This PR removes the LESS files for the Machine Learning plugin and replaces them with Sass.
Process taken
src/core_plugins/machine_learning/index.jsfile was updated to buildx-pack/plugins/ml/public/index.scssinto a css file.styling_constants.scssfromui/public/stylesin Kibana. This a placeholder file that includes all theming and invisible sass globals from EUI.x-pack/plugins/ml/public/...directory were replaced with sass counterparts._index.scssand_component_names.scssfiles next to the components or views they live next to.The selectors were all changed to match EUI's BEM formatting. This means the html/js templating was touched as well.Additionally, a three-letter prefixmchwas added to all selectors to namespace them and avoid conflicts.ML css / design layer cleanup plan of attack
#ml-appto sandbox.Major visual changes
I went ahead and made the job list table act similar to our other selections so that it didn't have the awkward gap to the left of the search area. Later, we should do a pass to use the button dropdown pattern to deal with sections + actions.