-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
cluster-ui: created a "connected" component for index details page #82174
Conversation
b78032e
to
2d8b1ef
Compare
868dfc1
to
4a53307
Compare
4a53307
to
0152871
Compare
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@cockroachlabs/cluster-ui", | |||
"version": "22.1.0-prerelease-3", | |||
"version": "22.1.0-prerelease-4", |
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.
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)
pkg/ui/workspaces/cluster-ui/package.json
line 3 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
@maryliag just bumped this to
22.2.0-prerelease-1
in #82476. Do you want this new component to be part of the 22.1 series, or will it go out in the fall?
Also, you should only change this value if you have published to npm, which I'm assuming you didn't do
}; | ||
|
||
export type ResetIndexUsageStatsRequest = cockroach.server.serverpb.ResetIndexUsageStatsRequest; | ||
export type ResetIndexUsageStatsResponse = cockroach.server.serverpb.ResetIndexUsageStatsResponse; |
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.
Do we need to export these request & response types? I don't see them being used.
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.
LGTM, just a couple of questions.
e8a41f4
to
a349767
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.
Addressed the comments so far, going to try to get these changes showing on CC console before merging anything.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)
pkg/ui/workspaces/cluster-ui/src/api/indexDetailsApi.ts
line 22 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Do we need to export these request & response types? I don't see them being used.
Yup, unexported both.
pkg/ui/workspaces/cluster-ui/package.json
line 3 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Also, you should only change this value if you have published to npm, which I'm assuming you didn't do
Reverted this change.
a349767
to
7ceb336
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.
Reviewed 7 of 13 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @matthewtodd)
42a0394
to
9f78199
Compare
This PR introduces an "indexDetailsConnected" component on cluster-ui. This allows us to reuse the cluster-ui redux store without replicating the same logic on CC console. Release note: None
9f78199
to
fa2213a
Compare
bors r+ |
Build failed (retrying...): |
Build failed (retrying...): |
Build succeeded: |
This PR introduces an "indexDetailsConnected" component on cluster-ui.
This allows us to reuse the cluster-ui redux store without replicating
the same logic on CC console.
Demo
"indexDetailsConnected" component being consumed by v22.2 Dedicated & Serverless clusters on CC console:
https://www.loom.com/share/f698510a5bd544618a6c9a8bbcf167d4
This is the PR that consumes this component in the demo.
Release note: None