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

Replace style with css prop in CSP package #202013

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Replace style with css prop in CSP package #202013

merged 1 commit into from
Dec 2, 2024

Conversation

albertoblaz
Copy link
Contributor

@albertoblaz albertoblaz commented Nov 27, 2024 •
edited by kibanamachine
Loading

Summary

Part of the resolution of this issue:

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.

Screenshots

Default Edge Screenshot 2024-12-02 at 16 27 47
Graph Popovers Screenshot 2024-12-02 at 16 27 57
Graph Stacked Edge Cases Screenshot 2024-12-02 at 16 28 03

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

  • Minor risk with low impact and severity:
    • Only risk is CSP graph nodes showing with a different background and border

@albertoblaz albertoblaz added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport:version Backport to applied version labels v8.18.0 labels Nov 27, 2024
@albertoblaz albertoblaz self-assigned this Nov 27, 2024
@albertoblaz albertoblaz requested a review from a team as a code owner November 27, 2024 15:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

@albertoblaz albertoblaz force-pushed the replace-style-packages-csp branch from 241318d to 3bda1cd Compare November 27, 2024 16:08
@albertoblaz albertoblaz enabled auto-merge (squash) November 27, 2024 20:32
@albertoblaz albertoblaz disabled auto-merge November 27, 2024 20:32
seanrathier
seanrathier approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@seanrathier seanrathier left a comment

Choose a reason for hiding this comment

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

Thanks for the updating this for us. You might need @kfirpeled to approve this since it is related to graphs.

@albertoblaz albertoblaz requested review from kfirpeled and a team November 29, 2024 15:00
opauloh
opauloh approved these changes Nov 30, 2024
View reviewed changes
Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

💯

@albertoblaz
Copy link
Contributor Author

@elasticmachine merge upstream

@albertoblaz albertoblaz mentioned this pull request Dec 2, 2024
Open
@kfirpeled
Copy link
Contributor

Hi @albertoblaz I found one issue within this PR

The handles of the nodes should not be visible however when I checked it with the graph storybooks:

yarn storybook cloud_security_posture_packages

You will see the following:

Screenshot 2024-12-02 at 15 30 19

kfirpeled
kfirpeled requested changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

using css for the handles doesn't override the default styles of the handles

@albertoblaz albertoblaz force-pushed the replace-style-packages-csp branch from cc9f2c1 to 09f36ae Compare December 2, 2024 15:13
@albertoblaz albertoblaz requested a review from kfirpeled December 2, 2024 15:27
kfirpeled
kfirpeled approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

lgtm, checked both using storybook and archived data

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #97 / discover/group4 adhoc data views should support query and filtering

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
securitySolution 14.6MB 14.6MB +139.0B

History

cc @albertoblaz

@albertoblaz albertoblaz merged commit a32d9c7 into elastic:main Dec 2, 2024
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12125149221

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 2, 2024
@albertoblaz
Replace style with css prop in CSP package (elastic#202013)
a773b31 
## 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.

### Screenshots

<details><summary>Default Edge</summary>
<img width="1028" alt="Screenshot 2024-12-02 at 16 27 47"
src="https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81">

</details>

<details><summary>Graph Popovers</summary>
<img width="175" alt="Screenshot 2024-12-02 at 16 27 57"
src="https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d">

</details>

<details><summary>Graph Stacked Edge Cases</summary>
<img width="1319" alt="Screenshot 2024-12-02 at 16 28 03"
src="https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c">
</details>

### Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

- [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

- Minor risk with low impact and severity:
- Only risk is CSP graph nodes showing with a different background and
border

(cherry picked from commit a32d9c7)
@kibanamachine kibanamachine mentioned this pull request Dec 2, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 2, 2024
@kibanamachine @albertoblaz
[8.x] Replace &#x60;style&#x60; with &#x60;css&#x60; prop in CSP pack…
3dc759f 
…age (#202013) (#202560)

# Backport

This will backport the following commits from `main` to `8.x`:
- [Replace &#x60;style&#x60; with &#x60;css&#x60; prop in CSP package
(#202013)](#202013)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alberto
Blázquez","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-02T17:58:18Z","message":"Replace
`style` with `css` prop in CSP package (#202013)\n\n##
Summary\r\n\r\nPart of the resolution of this issue: \r\n-
https://github.com/elastic/kibana/issues/149246\r\n\r\nRemoves the
`style` prop in React components and elements to avoid using\r\ninline
styles. Instead, it uses now the `emotion.css` prop to\r\ndynamically
attach all styles to the native `class` attribute.\r\n\r\n###
Motivation\r\n\r\nUsing inline styles at scale causes a performance
penalty at rendering\r\ntime. It's way more efficient to attach styles
to a single or several\r\nclassnames instead.\r\n\r\n###
Screenshots\r\n\r\n<details><summary>Default Edge</summary>\r\n<img
width=\"1028\" alt=\"Screenshot 2024-12-02 at 16 27
47\"\r\nsrc=\"https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81\">\r\n\r\n</details>
\r\n\r\n<details><summary>Graph Popovers</summary>\r\n<img width=\"175\"
alt=\"Screenshot 2024-12-02 at 16 27
57\"\r\nsrc=\"https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d\">\r\n\r\n</details>
\r\n\r\n<details><summary>Graph Stacked Edge Cases</summary>\r\n<img
width=\"1319\" alt=\"Screenshot 2024-12-02 at 16 28
03\"\r\nsrc=\"https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c\">\r\n</details>
\r\n\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following
conditions. \r\n\r\nReviewers should verify this PR satisfies this list
as well.\r\n\r\n- [x] The PR description includes the appropriate
Release Notes section,\r\nand the correct `release_note:*` label is
applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n###
Identify risks\r\n\r\n- Minor risk with low impact and severity:\r\n-
Only risk is CSP graph nodes showing with a different background
and\r\nborder","sha":"a32d9c782cefdb16c064f947ca10e49f3eb32cde","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Cloud
Security","backport:prev-minor","backport:version","v8.18.0"],"title":"Replace
`style` with `css` prop in CSP
package","number":202013,"url":"https://github.com/elastic/kibana/pull/202013","mergeCommit":{"message":"Replace
`style` with `css` prop in CSP package (#202013)\n\n##
Summary\r\n\r\nPart of the resolution of this issue: \r\n-
https://github.com/elastic/kibana/issues/149246\r\n\r\nRemoves the
`style` prop in React components and elements to avoid using\r\ninline
styles. Instead, it uses now the `emotion.css` prop to\r\ndynamically
attach all styles to the native `class` attribute.\r\n\r\n###
Motivation\r\n\r\nUsing inline styles at scale causes a performance
penalty at rendering\r\ntime. It's way more efficient to attach styles
to a single or several\r\nclassnames instead.\r\n\r\n###
Screenshots\r\n\r\n<details><summary>Default Edge</summary>\r\n<img
width=\"1028\" alt=\"Screenshot 2024-12-02 at 16 27
47\"\r\nsrc=\"https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81\">\r\n\r\n</details>
\r\n\r\n<details><summary>Graph Popovers</summary>\r\n<img width=\"175\"
alt=\"Screenshot 2024-12-02 at 16 27
57\"\r\nsrc=\"https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d\">\r\n\r\n</details>
\r\n\r\n<details><summary>Graph Stacked Edge Cases</summary>\r\n<img
width=\"1319\" alt=\"Screenshot 2024-12-02 at 16 28
03\"\r\nsrc=\"https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c\">\r\n</details>
\r\n\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following
conditions. \r\n\r\nReviewers should verify this PR satisfies this list
as well.\r\n\r\n- [x] The PR description includes the appropriate
Release Notes section,\r\nand the correct `release_note:*` label is
applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n###
Identify risks\r\n\r\n- Minor risk with low impact and severity:\r\n-
Only risk is CSP graph nodes showing with a different background
and\r\nborder","sha":"a32d9c782cefdb16c064f947ca10e49f3eb32cde"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202013","number":202013,"mergeCommit":{"message":"Replace
`style` with `css` prop in CSP package (#202013)\n\n##
Summary\r\n\r\nPart of the resolution of this issue: \r\n-
https://github.com/elastic/kibana/issues/149246\r\n\r\nRemoves the
`style` prop in React components and elements to avoid using\r\ninline
styles. Instead, it uses now the `emotion.css` prop to\r\ndynamically
attach all styles to the native `class` attribute.\r\n\r\n###
Motivation\r\n\r\nUsing inline styles at scale causes a performance
penalty at rendering\r\ntime. It's way more efficient to attach styles
to a single or several\r\nclassnames instead.\r\n\r\n###
Screenshots\r\n\r\n<details><summary>Default Edge</summary>\r\n<img
width=\"1028\" alt=\"Screenshot 2024-12-02 at 16 27
47\"\r\nsrc=\"https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81\">\r\n\r\n</details>
\r\n\r\n<details><summary>Graph Popovers</summary>\r\n<img width=\"175\"
alt=\"Screenshot 2024-12-02 at 16 27
57\"\r\nsrc=\"https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d\">\r\n\r\n</details>
\r\n\r\n<details><summary>Graph Stacked Edge Cases</summary>\r\n<img
width=\"1319\" alt=\"Screenshot 2024-12-02 at 16 28
03\"\r\nsrc=\"https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c\">\r\n</details>
\r\n\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following
conditions. \r\n\r\nReviewers should verify this PR satisfies this list
as well.\r\n\r\n- [x] The PR description includes the appropriate
Release Notes section,\r\nand the correct `release_note:*` label is
applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n###
Identify risks\r\n\r\n- Minor risk with low impact and severity:\r\n-
Only risk is CSP graph nodes showing with a different background
and\r\nborder","sha":"a32d9c782cefdb16c064f947ca10e49f3eb32cde"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Alberto Blázquez <[email protected]>
@albertoblaz albertoblaz deleted the replace-style-packages-csp branch December 3, 2024 08:20
hop-dev pushed a commit to hop-dev/kibana that referenced this pull request Dec 5, 2024
@albertoblaz @hop-dev
Replace style with css prop in CSP package (elastic#202013)
8c92b41 
## 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.

### Screenshots

<details><summary>Default Edge</summary>
<img width="1028" alt="Screenshot 2024-12-02 at 16 27 47"
src="https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81">

</details> 

<details><summary>Graph Popovers</summary>
<img width="175" alt="Screenshot 2024-12-02 at 16 27 57"
src="https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d">

</details> 

<details><summary>Graph Stacked Edge Cases</summary>
<img width="1319" alt="Screenshot 2024-12-02 at 16 28 03"
src="https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c">
</details> 


### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [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

- Minor risk with low impact and severity:
- Only risk is CSP graph nodes showing with a different background and
border
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 9, 2024
@albertoblaz @CAWilson94
Replace style with css prop in CSP package (elastic#202013)
26d51aa 
## 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.

### Screenshots

<details><summary>Default Edge</summary>
<img width="1028" alt="Screenshot 2024-12-02 at 16 27 47"
src="https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81">

</details> 

<details><summary>Graph Popovers</summary>
<img width="175" alt="Screenshot 2024-12-02 at 16 27 57"
src="https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d">

</details> 

<details><summary>Graph Stacked Edge Cases</summary>
<img width="1319" alt="Screenshot 2024-12-02 at 16 28 03"
src="https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c">
</details> 


### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [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

- Minor risk with low impact and severity:
- Only risk is CSP graph nodes showing with a different background and
border
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
@albertoblaz @CAWilson94
Replace style with css prop in CSP package (elastic#202013)
468cd08 
## 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.

### Screenshots

<details><summary>Default Edge</summary>
<img width="1028" alt="Screenshot 2024-12-02 at 16 27 47"
src="https://github.com/user-attachments/assets/4c913a69-ee26-4cda-829c-2b26799ead81">

</details> 

<details><summary>Graph Popovers</summary>
<img width="175" alt="Screenshot 2024-12-02 at 16 27 57"
src="https://github.com/user-attachments/assets/55054b05-9cb4-4ca7-a19a-319277d7961d">

</details> 

<details><summary>Graph Stacked Edge Cases</summary>
<img width="1319" alt="Screenshot 2024-12-02 at 16 28 03"
src="https://github.com/user-attachments/assets/11ec7a03-e8cf-4090-9443-56288bf78a2c">
</details> 


### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [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

- Minor risk with low impact and severity:
- Only risk is CSP graph nodes showing with a different background and
border
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfirpeled kfirpeled kfirpeled approved these changes

@opauloh opauloh opauloh approved these changes

@seanrathier seanrathier seanrathier approved these changes

Assignees

@albertoblaz albertoblaz

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@albertoblaz @elasticmachine @kfirpeled @kibanamachine @seanrathier @opauloh