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.

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

Migrate and cleanup legacy scss #69369

Merged
merged 42 commits into from
Jul 20, 2020

Conversation

kertal
Copy link
Member

@kertal kertal commented Jun 17, 2020

Summary

Cleanup and migrate legacy scss of src/legacy/ui/public/styles/_legacy/

  • Deletes unused CSS
  • Moves single plugin CSS usage to the consuming plugin
  • Migrated base layout css to src/core/public/styles

Fixes #69135

@kertal
Copy link
Member Author

kertal commented Jun 17, 2020

@elasticmachine merge upstream

@kertal kertal self-assigned this Jul 6, 2020
@kertal
Copy link
Member Author

kertal commented Jul 7, 2020

@elasticmachine merge upstream

@kertal kertal marked this pull request as ready for review July 14, 2020 11:30
@kertal kertal removed request for a team July 16, 2020 07:32
@kertal
Copy link
Member Author

kertal commented Jul 16, 2020

I'm getting an error in Timelion trying to start this up to test it. Looks to be a bad .scss import in src/plugins/timelion/public/directives/_index.scss with the saved_object_finder import.

@crob611 thx, yes, didn't work, fixed it, and due to changes, the canvas team review is no longer required. but of course you could have a look, that everything is fine :)

@kertal
Copy link
Member Author

kertal commented Jul 16, 2020

The main thing I'm concerned about here is that the application styles that determine the layout of the whole page (and some chrome styles) are now under the "legacy" style import but really I think all applications need it, even they do override it

@kertal If there are base styles used by everything, I think it's fair moving them into core. @joshdover do you think that's the right approach here? I guess it's mostly about this stuff: #69369 (files)

@joshdover @flash1293 So for now, should I move them to e.g. src/legacy/ui/public/styles/_base.scss? until you move them to src/core/public/styles?

I have no problem with you moving these directly into src/core/public/styles, but we can also manage moving them in #58369 if that is easier.

@joshdover I've migrated those style to src/core/public/styles

@kertal kertal requested review from cchaos and walterra July 16, 2020 16:02
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Ahhh, that feels better. Thanks! Just one comment about a duplicate file but otherwise LGTM

@kertal kertal requested review from a team and flash1293 July 16, 2020 19:19
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Firefox, focusing on Timelion, Discover and data table vis. Didn't notice any breakages, LGTM.

Thanks for this PR, this is one of the very last Kibana app legacy platform parts 💯 🥇

@kertal
Copy link
Member Author

kertal commented Jul 19, 2020

@elasticmachine merge upstream

1 similar comment
@kertal
Copy link
Member Author

kertal commented Jul 20, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
kibanaLegacy 95 +9 86

async chunks size

id value diff baseline
discover 431.2KB +2.9KB 428.3KB
home 510.5KB -356.0B 510.9KB
kibanaLegacy 147.1KB -6.0KB 153.1KB
security 1.1MB -712.0B 1.1MB
spaces 45.6KB -356.0B 45.9KB
timelion 591.1KB +8.7KB 582.3KB
total - +4.2KB -

page load bundle size

id value diff baseline
core 1.1MB -95.5KB 1.2MB
kibanaLegacy 246.8KB +14.2KB 232.6KB
ml 4.3MB -4.5KB 4.3MB
visTypeTable 604.6KB +184.0B 604.4KB
total - -85.7KB -

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

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.

ML edits LGTM

@kertal kertal merged commit ec4f9d5 into elastic:master Jul 20, 2020
kertal added a commit to kertal/kibana that referenced this pull request Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate legacy styles
10 participants