Skip to content

[Defend workflows] [On-week] Migrate osquery from styled-components to @emotion#161179

Merged
tomsonpl merged 38 commits intoelastic:mainfrom
tomsonpl:migrate-styles-osquery
Aug 2, 2023
Merged

[Defend workflows] [On-week] Migrate osquery from styled-components to @emotion#161179
tomsonpl merged 38 commits intoelastic:mainfrom
tomsonpl:migrate-styles-osquery

Conversation

@tomsonpl
Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl commented Jul 4, 2023 •
edited
Loading

Notes:

  • Added euiTheme globally to emotion.d.ts
  • Removed styled-components from osquery plugin.
  • Wrapped CasesProvider with styled components themeProvider because eg. cases UI is used from osquery

@tomsonpl tomsonpl added chore release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Feature:Osquery Security Solution Osquery feature v8.10.0 labels Jul 4, 2023
@tomsonpl tomsonpl self-assigned this Jul 4, 2023
tomsonpl
tomsonpl commented Jul 6, 2023
View reviewed changes
x-pack/plugins/cases/public/components/cases_context/index.tsx
);

return isCasesContextValue(value) ? (
<QueryClientProvider client={casesQueryClient}>
Copy link
Copy Markdown
Contributor Author

@tomsonpl tomsonpl Jul 6, 2023

Choose a reason for hiding this comment

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

this had to be adjusted - otherwise using cases.ui ends up not finding theme in case where original plugin does not provide it - eg. Osquery plugin (previously had styled-components provider) using emotion provider, renders cases.ui addToCaseModal = it crashed because of missing theme.

Copy link
Copy Markdown
Member

@cnasikas cnasikas Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @tomsonpl!

@tomsonpl tomsonpl marked this pull request as ready for review July 6, 2023 18:02
@tomsonpl tomsonpl requested review from a team as code owners July 6, 2023 18:02
cee-chen
cee-chen reviewed Jul 7, 2023
View reviewed changes
@tomsonpl tomsonpl mentioned this pull request Jul 10, 2023
Merged
5 tasks
cee-chen
cee-chen reviewed Aug 1, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Mostly optional / cleanup comments! As I mentioned in Slack, this is just code review on my part, no QA :)

x-pack/plugins/osquery/public/saved_queries/form/playground_flyout.tsx
Comment on lines +15 to +20
const euiFlyoutHeaderCss = {
'&.euiFlyoutHeader': {
paddingTop: '21px',
paddingBottom: '20px',
},
};
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Mentioning for completeness, @elastic/kibana-design designers (in the past) generally encouraged devs to use euiTheme.size variables rather than use static px values. For the purpose of this conversion I don't think it's required since it was already that way before, but maybe something to think about later!

@tomsonpl tomsonpl requested a review from a team as a code owner August 1, 2023 11:34
@tomsonpl tomsonpl removed the request for review from a team August 2, 2023 08:11
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Aug 2, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
osquery 265 282 +17

Async chunks

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

id before after diff
cases 421.2KB 421.2KB -1.0B
osquery 1.0MB 1.0MB -2.1KB
total -2.1KB

Page load bundle

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

id before after diff
cases 151.2KB 151.3KB +84.0B
osquery 51.9KB 51.6KB -291.0B
total -207.0B
Unknown metric groups

async chunk count

id before after diff
osquery 12 13 +1

ESLint disabled line counts

id before after diff
osquery 107 110 +3

Total ESLint disabled count

id before after diff
osquery 108 111 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tomsonpl

@tomsonpl tomsonpl removed the request for review from a team August 2, 2023 12:31
cee-chen
cee-chen approved these changes Aug 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🎉 🎉 Thanks again for being super proactive on moving from styled-components to Emotion! I appreciate y'all!

@tomsonpl tomsonpl merged commit ffb716d into elastic:main Aug 2, 2023
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrykkopycinski patrykkopycinski patrykkopycinski approved these changes

@cnasikas cnasikas cnasikas approved these changes

@Ikuni17 Ikuni17 Ikuni17 approved these changes

@szwarckonrad szwarckonrad Awaiting requested review from szwarckonrad

+1 more reviewer

@cee-chen cee-chen cee-chen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tomsonpl tomsonpl

Labels

backport:skip This PR does not require backporting chore Feature:Osquery Security Solution Osquery feature release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.10.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tomsonpl @elasticmachine @kibana-ci @cee-chen @patrykkopycinski @cnasikas @Ikuni17 @kibanamachine