-
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
Migrate rollup client side code #63227
Conversation
💔 Build Failed
Failed CI StepsHistory
To update your PR or re-run it, just comment with: |
Pinging @elastic/kibana-app (Team:KibanaApp) |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Code LGTM and tested the Rollup Jobs management app locally. Note that I haven't tested rollup index patterns. My main gripe is with getUiStatsReporter
-- I'd much prefer an imperative API like trackUiMetric
. :)
Thanks for doing this @flash1293!
// @ts-ignore | ||
import { rollupJobsStore } from './crud_app/store'; | ||
import { rollupJobsStore } from './crud_app/store/index'; |
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.
Nit: We can leave off the /index
part of this path. Webpack will automatically resolve to an index.ts
or index.js
file.
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.
Those were introduced by Webstorm when moving files - I'm removing them.
x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js
Outdated
Show resolved
Hide resolved
@@ -263,7 +264,7 @@ export class JobTable extends Component { | |||
content = ( | |||
<EuiLink | |||
onClick={() => { | |||
trackUiMetric(METRIC_TYPE.CLICK, UIM_SHOW_DETAILS_CLICK); | |||
getUiStatsReporter()(METRIC_TYPE.CLICK, UIM_SHOW_DETAILS_CLICK); |
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.
Minor nit: the original form of calling a function like trackUiMetric
makes this code seem more readable to me, because the code describes what we're doing. In this new form, it says we get a UI stats reporter, which sounds like a noun to me, which obscures the action we're performing.
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 re-introduced a helper in that format hiding the getter in the services module.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses·ts.detection engine api security and spaces enabled find_statuses should return a single rule status when a single rule is loaded from a find status with defaults addedStandard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
This PR moves the client side part of the
rollup
plugin into the new platform.Besides just moving files around, this PR also includes:
ui/new_platform
)ui/new_platform
as well)