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

[Reporting] Remove preserve_layout injected css #126475

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Feb 28, 2022

Summary

Closes #120348

This PR removes the global injection of app-specific styles in Screenshotting in preserve_layout except for two rules:

.hide-for-sharing {}

#globalBannerList {}

Both of which may introduce changes that are beyond the scope of this PR:

  • .hide-for-sharing is used in a number of places, but can easily be removed and driven by JS (screenshotMode)
  • #globalBannerList is perhaps slightly trickier since that code does not live inside of plugins (lives in core). We may have to leave this for now, although it is only present in preserve_layout screenshots. Perhaps it could safely be removed?

The primary motivation behind this is to move related code closer together, and out of Reporting which should not know anything about these apps. Functionally, this PR should not change anything. We should still have the same styles as before when taking screenshots (please test!). Per @tsullivan - we do have functional tests that should give more peace-of-mind for this refactor.

Additional notes

  • In the future, we would like to experiment with introducing print media views for apps, this work will make it easier to work with apps in an isolated way.

@jloleysens jloleysens added chore (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppServicesSv release_note:skip Skip the PR/issue when compiling release notes v8.2.0 labels Feb 28, 2022
@jloleysens jloleysens requested review from a team as code owners February 28, 2022 15:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

/**
* global
* Global utilities
*/

/* elements can hide themselves when shared */
.hide-for-sharing {
Copy link
Contributor Author

@jloleysens jloleysens Feb 28, 2022

Choose a reason for hiding this comment

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

We can open up a follow-up issue to address this.

Copy link
Member

Choose a reason for hiding this comment

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

I think what you wrote about just doing this in JS within the screenshotting plugin is a good idea

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visualizations 209 214 +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visualizations 157.8KB 163.9KB +6.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visualizations 43.9KB 44.1KB +143.0B
Unknown metric groups

async chunk count

id before after diff
visualizations 10 11 +1

History

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

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 and everything still seems to work fine

@tsullivan
Copy link
Member

We should still have the same styles as before when taking screenshots (please test!).

There are a few automated tests that ensure that the latest PNG reports visually match a baseline image, which should give a lot of confidence here :)

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

@jloleysens jloleysens merged commit d53154e into elastic:main Mar 2, 2022
@jloleysens jloleysens deleted the reporting/remove-preserve-layout-injected-css branch March 2, 2022 12:58
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 4, 2022
@tsullivan tsullivan added the backport:skip This commit does not require backporting label Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@elastic elastic deleted a comment from kibanamachine Mar 16, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting chore (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting] Remove CSS for "preserve_layout"
7 participants