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

migrate style overrides from scss to leverage emotion #204463

Merged
merged 2 commits into from
Dec 17, 2024
Merged

migrate style overrides from scss to leverage emotion #204463

merged 2 commits into from
Dec 17, 2024

Conversation

eokoneyo
Copy link
Contributor

Summary

This PR migrates style overrides within core from SCSS to leverage emotion. Why is this necessary? Kibana builds it's stylesheets we get 4 stylesheets for all the supported themes, in which in turn impacts the files built. Switching to leveraging emotion makes it such that we don't need to build 4 files anymore for this particular cases in turn reducing the page load bundle size.

This PR results in ~12% reduction in the page load bundle, that would have come into effect from the PR that introduces the new Borealis theme as a default. The results can be seen in this build

@eokoneyo eokoneyo self-assigned this Dec 16, 2024
@eokoneyo eokoneyo requested review from a team as code owners December 16, 2024 18:03
@eokoneyo eokoneyo added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) EUI Visual Refresh labels Dec 16, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@eokoneyo eokoneyo added backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) backport:skip This commit does not require backporting and removed backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Dec 16, 2024
@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #7 / StepSelectHosts should display dropdown with agent policy selected when Existing hosts selected

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 245 246 +1
core 435 436 +1
esUiShared 221 222 +1
fleet 1250 1251 +1
observability 1107 1108 +1
securitySolution 6410 6411 +1
triggersActionsUi 868 869 +1
total +7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-application-common 1 2 +1

Async chunks

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

id before after diff
observability 480.8KB 480.8KB +16.0B

Page load bundle

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

id before after diff
core 450.9KB 449.1KB -1.8KB
esUiShared 106.9KB 106.9KB +4.0B
fleet 173.8KB 173.8KB +21.0B
total -1.7KB
Unknown metric groups

API count

id before after diff
@kbn/core-application-common 8 9 +1

cc @eokoneyo

ryankeairns
ryankeairns approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Design review.
The styles look to all be moved over; ran the app locally, with a banner, and things seem to layout and behave correctly (nav old and new, flyouts, etc.)

@eokoneyo eokoneyo merged commit 26fe24a into elastic:main Dec 17, 2024
8 checks passed
@eokoneyo eokoneyo deleted the chore/migrate-scss-files-core-to-leverage-emotion branch December 17, 2024 09:22
@mgadewoll
Copy link
Contributor

I'm too late to the party but anyway changes LGTM from EUI side ✅

JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
@eokoneyo @elasticmachine @JoseLuisGJ
migrate style overrides from scss to leverage emotion (elastic#204463)
52a0a45 
## Summary

This PR migrates style overrides within core from SCSS to leverage
emotion. Why is this necessary? Kibana builds it's stylesheets we get 4
stylesheets for all the supported themes, in which in turn impacts the
files built. Switching to leveraging emotion makes it such that we don't
need to build 4 files anymore for this particular cases in turn reducing
the page load bundle size.

This PR results in ~12% reduction in the page load bundle, that would
have come into effect from [the
PR](elastic#203840) that introduces the
new Borealis theme as a default. The results can be seen in [this
build](https://buildkite.com/elastic/kibana-pull-request/builds/260881)

<!--

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
@eokoneyo @elasticmachine @CAWilson94
migrate style overrides from scss to leverage emotion (elastic#204463)
3229a92 
## Summary

This PR migrates style overrides within core from SCSS to leverage
emotion. Why is this necessary? Kibana builds it's stylesheets we get 4
stylesheets for all the supported themes, in which in turn impacts the
files built. Switching to leveraging emotion makes it such that we don't
need to build 4 files anymore for this particular cases in turn reducing
the page load bundle size.

This PR results in ~12% reduction in the page load bundle, that would
have come into effect from [the
PR](elastic#203840) that introduces the
new Borealis theme as a default. The results can be seen in [this
build](https://buildkite.com/elastic/kibana-pull-request/builds/260881)

<!--

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@tsullivan tsullivan added the technical debt Improvement of the software architecture and operational architecture label Jan 21, 2025
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
@eokoneyo @elasticmachine @viduni94
migrate style overrides from scss to leverage emotion (elastic#204463)
3185a05 
## Summary

This PR migrates style overrides within core from SCSS to leverage
emotion. Why is this necessary? Kibana builds it's stylesheets we get 4
stylesheets for all the supported themes, in which in turn impacts the
files built. Switching to leveraging emotion makes it such that we don't
need to build 4 files anymore for this particular cases in turn reducing
the page load bundle size.

This PR results in ~12% reduction in the page load bundle, that would
have come into effect from [the
PR](elastic#203840) that introduces the
new Borealis theme as a default. The results can be seen in [this
build](https://buildkite.com/elastic/kibana-pull-request/builds/260881)

<!--

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afharo afharo afharo approved these changes

@ryankeairns ryankeairns ryankeairns approved these changes

Assignees

@eokoneyo eokoneyo

Labels
backport:skip This commit does not require backporting EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) technical debt Improvement of the software architecture and operational architecture v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eokoneyo @elasticmachine @mgadewoll @ryankeairns @afharo @tsullivan @kibanamachine