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

[8.x] Replace inline styles with `css` prop in `src/` folder (#201563) #202460

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

kibanamachine
Copy link
Contributor

Backport

This will backport the following commits from main to 8.x:

Questions ?

Please refer to the Backport tool documentation

## Summary

Part of the resolution of this issue:
- elastic#149246

Removes the `style` prop in React components and elements to avoid using
inline styles. Instead, it uses now the `emotion.css` prop to
dynamically attach all styles to the native `class` attribute.

### Motivation

Using inline styles at scale causes a performance penalty at rendering
time. It's way more efficient to attach styles to a single or several
classnames instead.

### How to review

From [Emotion's official
docs](https://emotion.sh/docs/css-prop#style-precedence):

> [!NOTE]
> Any component or element that accepts a `className` prop can also use
the `css` prop. The styles supplied to the `css` prop are evaluated and
the computed class name is applied to the `className` prop.

Components that are safe to migrate from `style` to `css`:
- React elements
- React components exposed by EUI (they all support a `className` prop
and the [Babel
plugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is
enabled throughout Kibana)

Components where styling might break:
- Custom component we wrote that currently don't accept a `className`
prop.
- Specific 3P components that pass in a `style` prop to our components
i.e. `react-window` (it calculates the dynamic position of all
virtualized elements and hides the ones outside the visible fold)
- Specific 3P components that expect a `style` prop to be styled i.e.
charts, etc.

### Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

- [x] [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
- [x] 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.
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] 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

Main risk is breaking the UI by not applying or incorrectly applying the
styling. This was explained in the "How to review" section above. The
risk has **low severity** though. The mitigation plan will simply be
reverting the commit asap not to affect production and test locally
until it's fixed./

(cherry picked from commit 6aaecd0)
@kibanamachine kibanamachine enabled auto-merge (squash) December 2, 2024 13:21
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Dec 2, 2024
@kibanamachine kibanamachine merged commit 97e8759 into elastic:8.x Dec 2, 2024
14 checks passed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
console 206.5KB 206.6KB +168.0B
controls 459.3KB 459.4KB +67.0B
data 52.4KB 52.5KB +36.0B
dataViewEditor 49.4KB 49.4KB +61.0B
dataViewFieldEditor 177.7KB 177.8KB +116.0B
dataViewManagement 139.6KB 139.8KB +143.0B
devTools 7.7KB 7.8KB +19.0B
discover 811.2KB 811.3KB +132.0B
expressionError 14.3KB 14.4KB +62.0B
expressionLegacyMetricVis 11.9KB 12.0KB +23.0B
expressionMetricVis 5.2KB 5.2KB -17.0B
expressionPartitionVis 35.6KB 35.6KB +32.0B
expressionRepeatImage 1.2KB 1.2KB +28.0B
expressionXY 127.7KB 127.8KB +83.0B
home 151.9KB 152.0KB +19.0B
imageEmbeddable 69.0KB 69.0KB +43.0B
inputControlVis 52.1KB 52.1KB +29.0B
inspector 28.3KB 28.3KB +28.0B
presentationPanel 15.3KB 15.4KB +38.0B
presentationUtil 87.4KB 77.1KB -10.3KB
savedObjectsManagement 86.6KB 86.6KB +33.0B
uiActionsEnhanced 136.2KB 136.4KB +239.0B
unifiedDocViewer 126.1KB 126.1KB +30.0B
unifiedSearch 356.3KB 356.5KB +229.0B
visDefaultEditor 95.3KB 95.3KB +29.0B
visTypeTimeseries 507.0KB 507.1KB +110.0B
visualizations 318.3KB 318.4KB +77.0B
total -8.5KB

Page load bundle

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

id before after diff
charts 45.3KB 45.3KB +32.0B
dataViewFieldEditor 26.8KB 26.8KB +36.0B
esqlDataGrid 9.3KB 9.4KB +38.0B
expressions 100.3KB 100.3KB +27.0B
guidedOnboarding 29.6KB 29.6KB +30.0B
interactiveSetup 59.6KB 59.7KB +128.0B
kibanaReact 39.4KB 39.6KB +202.0B
presentationUtil 31.6KB 31.7KB +22.0B
visDefaultEditor 24.0KB 24.0KB +23.0B
total +538.0B

cc @albertoblaz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants