Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML] convert Less to Scss #25574

Merged
merged 6 commits into from
Nov 15, 2018
Merged

[ML] convert Less to Scss #25574

merged 6 commits into from
Nov 15, 2018

Conversation

snide
Copy link
Contributor

@snide snide commented Nov 13, 2018 •
edited
Loading

Replaces #23387 and is rebased against master

This PR removes the LESS files for the Machine Learning plugin and replaces them with Sass.

Process taken

  1. The src/core_plugins/machine_learning/index.js file was updated to build x-pack/plugins/ml/public/index.scss into a css file.
    • That index file imports styling_constants.scss from ui/public/styles in Kibana. This a placeholder file that includes all theming and invisible sass globals from EUI.
    • Any sass files under that line can now use the functions, mixins and variables from EUI. Although the theme is hardcoded, it will be relatively easy to switch themes based on this import later.
  2. All Less files in the x-pack/plugins/ml/public/... directory were replaced with sass counterparts.
    • The sass files now live with separated _index.scss and _component_names.scss files next to the components or views they live next to.
    • The Less files were then deleted entirely.
    • The new sass files use EUI variables whenever possible. The most important being color and sizing variables.
    • 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 prefix mch was added to all selectors to namespace them and avoid conflicts.

ML css / design layer cleanup plan of attack

  1. THIS PR: Convert ML Less to Sass
    • Straight conversion, try not to touch the template layer, don't focus on design
    • Introduce EUI variables where easy, but avoid anything major. Hex values and pixels still remain in spots.
    • Don't focus on selectors. Use #ml-app to sandbox.
  2. PR 2: Introduce EUI style naming and prefixes
    • Replace selectors across ML, prefix everything to avoid conflicts. Remove sandboxing.
  3. PR 3: Remove EUI overwrites in ML and replace with unique selectors
    • Remove all the copy/paste CSS and try to cut the footprint down
    • Remove and replace the EUI overwrites with something less brittle
  4. PR 3+: Design
    • Potentially do a similar reskinning like we did with canvas. Likely a large project.
    • Could be done in pieces as pages are converted to React so design team can step in to help

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.

image

@snide
Copy link
Contributor Author

snide commented Nov 13, 2018

@peteharverson and @cchaos. To make the rebase a little easier I squashed the previous PR down to a single commit. It is now up to date against master. I'll need another day to address the lingering issues from Pete's last review, but will have this ready for a final check on Wednesday so ML has some time to review before a Friday merge (after 7 FF).

@snide snide mentioned this pull request Nov 13, 2018
Closed
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sophiec20 sophiec20 added the :ml label Nov 13, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@sophiec20 sophiec20 changed the title Machine learning convert Less to Scss [ML] convert Less to Scss Nov 13, 2018
@snide
Copy link
Contributor Author

snide commented Nov 13, 2018

OK. Was worried why the +/- on the PR of this was so different from the last one and it was because jobs_list_old was removed since then. As a summary of what's been done in this PR past the original.

  • Merged the original 17 commits down to one.
  • Refactored the Less additions and changes (mostly around the explorer_chart) and made them work with the Sass.
  • Removed the jobs_list_old conversion work I'd done.
  • Fixed the last visual ideas reported in the Google doc review.

I'll do some more testing tonight (I'm in MV for xschool) but I think this is mostly good if @peteharverson wants to give it another check.

@snide snide requested a review from peteharverson November 13, 2018 23:06
@elasticmachine
Copy link
Contributor

💔 Build Failed

walterra
walterra requested changes Nov 14, 2018
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments, please check the deprecated paginated_table directive. Also added some comment/screenshots to the Google Doc, see New issues - 14 Nov. Almost there :)

x-pack/plugins/ml/public/components/paginated_table/_index.scss Outdated Show resolved Hide resolved
x-pack/plugins/ml/public/components/paginated_table/_paginated_table.scss Outdated Show resolved Hide resolved
x-pack/plugins/ml/public/components/paginated_table/paginated_table.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@snide snide mentioned this pull request Nov 15, 2018
Open
snide added 5 commits November 14, 2018 17:08
@snide
ml less to sass
092d968 
add in new css compile system

selector protect kibana from ml

introduce ML variables

timeseries explorer

forecasting modal

import job wizard sass files correctly

missing import for event rate chart

more feedback fixes

fix for chart overflow

ml code cleanup

code cleanup

more review

forgot to delete some less files

feedback

remove last of less files

bad import
@snide
replace and reworks less file changes in the gap period between origi…
8128f0a 
…nal PR and master
@snide
remove jobs list old content
7d3c937
@snide
remove paginated table
5a21ff8
@snide
feedback
fb6c7b0
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra
Copy link
Contributor

LGTM with latest changes! 💯

peteharverson
peteharverson approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Many thanks for doing this @snide

@snide snide merged commit bfa852a into elastic:master Nov 15, 2018
@snide snide deleted the sass/ml-rebase branch November 15, 2018 15:23
@snide snide mentioned this pull request Nov 15, 2018
Merged
snide added a commit to snide/kibana that referenced this pull request Nov 15, 2018
@snide
[ML] convert Less to Scss (elastic#25574)
d5ff0c2 
Coverts ML's Less files to Sass and introduces EUI variables.
snide added a commit that referenced this pull request Nov 15, 2018
@snide
[ML] convert Less to Scss (#25574) (#25745)
7d13cd6 
Coverts ML's Less files to Sass and introduces EUI variables.
This was referenced Nov 16, 2018
Merged
Merged
Merged
@peteharverson peteharverson mentioned this pull request Nov 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophiec20 sophiec20 sophiec20 left review comments

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra walterra approved these changes

@jgowdyelastic jgowdyelastic Awaiting requested review from jgowdyelastic

Assignees
No one assigned
Labels
:ml
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@snide @elasticmachine @walterra @sophiec20 @peteharverson