-
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
[Lens] Remove scss from annotations plugin, visualization-ui-components and gauge expression #208891
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
d2cd1ac
to
21bf098
Compare
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.
Thanks, @mbondyra. I've left two small comments/question regarding whether we can reduce some style repetition/redundancy, but this looks good to me otherwise. Assuming you're able to address those, I'll approve now so I don't hold you up.
css={css` | ||
& + .lnsRowCompressedMargin { | ||
margin-top: ${euiThemeVars.euiSizeS}; | ||
} | ||
`} |
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.
This same style is used multiple times throughout this PR. To keep things DRY, would it be better to store this style in a separate .style.ts
file and reference that one style in all locations, rather than repeating the style?
css: { | ||
color: !fieldOption.compatible ? euiThemeVars.euiColorLightShade : undefined, | ||
backgroundColor: !fieldOptionExists ? euiThemeVars.euiColorLightestShade : undefined, | ||
}, |
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.
Same comment here as previous. Is there anything that can be done here to avoid repeating the same styles?
21bf098
to
56d998a
Compare
} from '@elastic/eui'; | ||
import type { DataViewBase, Query } from '@kbn/es-query'; | ||
import { css } from '@emotion/react'; | ||
import { euiThemeVars } from '@kbn/ui-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.
It seems like the EUI team is asking people to move away from using euiThemeVars
and instead using the useEuiTheme
hook. See #199715 and search for euiThemeVars
.
Before:
import { EuiIcon, EuiCode, EuiText, useEuiTheme } from '@elastic/eui';
height: ${euiThemeVars.euiSizeXS} / 2;
After:
import { useEuiTheme } from '@elastic/eui'
const { euiTheme } = useEuiTheme();
height: ${euiTheme.size.xs} / 2;
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
History
|
Summary
part of #208908
Replaces scss to css-in-js. I've tested all the changes.