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

[Core] Clarity regarding bundling and loading of different plugins and their styles #138102

Closed
logeekal opened this issue Aug 4, 2022 · 6 comments
Labels
bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team

Comments

@logeekal
Copy link
Contributor

logeekal commented Aug 4, 2022

It is a UI issue so only kibana version matter.

Kibana version: <=8.4

Describe the bug:

While working on a bug I observed that styles of each plugin is loaded when the user visits that plugin (i.e. when that plugin is loaded) and not all at once. It creates inconsistent Dev and user experience.

For example, styles of observability will be applied to the app when a user visits any page within Observability and same goes for the the security.

Because of this, when doing the development, it becomes difficult to know if styles from any other plugin are conflicting with our own. Please see aforementioned bug as a use case here:

Please checkout related PR : #138091

Steps to reproduce:

This bug is reproducible today in kibana but we will soon be moving fix for the same. I guess it could still be repoduced in https://kibana.siem.estc.dev/ for some time. I will also add the recording here so that once the bug is fixed, we have a record.

  1. Visit Security --> Alerts in https://kibana.siem.estc.dev/ ( or your local kibana instance) (Preferably new window)
  2. We do not need any data for this test so do not worry, in case there is no data available.
  3. Open Untitled Timeline from the bottom drawer
  4. Click on the full screen icon open timeline in full screen.
  5. You will see the correct rendering of timeline going full screen
  6. Now visit, Observability --> Alerts
  7. Now, repeat steps 1 -> 4

Expected behavior:
There can 2 expected behaviours:

  1. Styles from plugins are loaded together so that development can be done with predictable UI interactions. But I am not sure if it is possible since, we are not bundling css separately ( I guess. Please correct me if I am wrong).

  2. Styles from one plugin is loaded and when user visits other plugin, styles from that plugin are also loaded and styles from all other plugins are purged so that there is not issue.

Screenshots (if relevant):
Please find the recording below:

  1. At the end of recording, I highlight a css, which is available below. As you can see the code is from observability plugin css but it is impacting security solution flyout and error occurs only when I have already visied Observability plugin ( since it is loaded lazily)

.euiFlyout,
.euiCollapsibleNav {
top: $fullscreenFlyoutTop;
height: calc(100% - #{$fullscreenFlyoutTop});

Screen.Recording.2022-08-04.at.13.59.02.mov

Observations

This happens because Alerts (Security) & Alerts (Observability) have conflicting styles and issue only occurs if a user has visited both of the pages.

It seems to me that we inject styles directly into js and do not generate separate css bundles. Please correct me if I am wrong. I think this will make it harder to load css all at once for complete application.

@logeekal logeekal added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team labels Aug 4, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@spalger
Copy link
Contributor

spalger commented Aug 4, 2022

We use the style-loader to support loading CSS when it is imported by code, this means that once styles are loaded they can't be unloaded, which can definitely lead to this type of problem but is far simpler to implement. I think the most correct solution here would be to migrate to Emotion, which is happening on some teams now and will spread to more teams as time goes on. In the meantime I think it's probably just an issue of properly scoping CSS rules because we're all sharing a single browser window and our CSS rules need to be properly scoped to the application they are intended to effect.

@logeekal
Copy link
Contributor Author

logeekal commented Aug 9, 2022

Thank you. This does make sense and good to know that we are moving towards emotion.

@pgayvallet
Copy link
Contributor

Should we therefore close this?

@logeekal
Copy link
Contributor Author

logeekal commented Aug 22, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team
Projects
None yet
Development

No branches or pull requests

4 participants