-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Replace style
with css
in CSP plugin
#202487
base: main
Are you sure you want to change the base?
Replace style
with css
in CSP plugin
#202487
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
bc09a92
to
d50e01f
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
cc @albertoblaz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
@@ -24,6 +25,7 @@ interface ChartPanelProps { | |||
isError?: boolean; | |||
rightSideItems?: ReactNode[]; | |||
styles?: React.CSSProperties; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we still need the styles if we are using css
?
@@ -24,6 +25,7 @@ interface ChartPanelProps { | |||
isError?: boolean; | |||
rightSideItems?: ReactNode[]; | |||
styles?: React.CSSProperties; | |||
css?: Interpolation<Theme>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw how we are using this prop here, can we use a size
prop instead if there are other components using this with different sizes?
css?: Interpolation<Theme>; | |
size?: 'small' | 'medium' | `large`; |
@@ -104,7 +104,7 @@ const TotalIntegrationsCount = ({ | |||
pageCount, | |||
totalCount, | |||
}: Record<'pageCount' | 'totalCount', number>) => ( | |||
<EuiText size="xs" style={{ marginLeft: 8 }}> | |||
<EuiText size="xs" css={{ marginLeft: 8 }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use an euiTheme.size
here?
@@ -87,12 +87,14 @@ const CounterButtonLink = ({ | |||
<EuiText | |||
color={color} | |||
css={css` | |||
font-weight: ${euiTheme.font.weight.medium}; | |||
font-size: 18px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use an euiTheme.size
here? ( or the typography equivalent)
@@ -72,7 +72,7 @@ const AccountDropDown = ({ | |||
options: Array<{ value: string; label: string }>; | |||
}) => ( | |||
<EuiComboBox | |||
style={{ width: 320 }} | |||
css={{ width: 320 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this responsive?
If it's not we should raise this in a follow-up issue to use breakpoints if we can.
Summary
Part of the resolution of this issue:
style
withemotion.css
#149246Removes the
style
prop in React components and elements to avoid using inline styles. Instead, it uses now theemotion.css
prop to dynamically attach all styles to the nativeclass
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.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:*
label is applied per the guidelines