Skip to content
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

ui: new insights table component #85862

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Aug 9, 2022

Creation of the base insights table,
that can be used on both schema insights and statement
details page.

This commit only introduce the table, with the different
types, but still needs the proper cell formating for
each type.

Part of #83783

Release note: None

@maryliag maryliag requested a review from a team August 9, 2022 22:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maryliag maryliag force-pushed the insight-component branch 5 times, most recently from 3ff18e8 to 1f02f38 Compare August 10, 2022 01:58
Copy link

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)


pkg/ui/workspaces/cluster-ui/src/indexRecommendationsTable/indexRecommendationsTable.tsx line 31 at r1 (raw file):

}

export class IdxInsightsSortedTable extends SortedTable<IdxRecommendation> {}

I see on the figma (https://www.figma.com/file/xpnRkP9cLXERVceRC4cec4/22.2_SQL-obsrv_query-performance?node-id=301%3A3805) there could be schema insights unrelated to index recs like Outdated Table Statistics, despite that, are we we okay with keeping this table as "Index Insights" ?


pkg/ui/workspaces/cluster-ui/src/indexRecommendationsTable/indexRecommendationsTable.tsx line 73 at r1 (raw file):

      return "Create New Index";
    case "DROP":
      return "Drop Index";

I think should be Drop Unused Index, currently that's the only index recommendation we recommend for index drops.

Creation of the base insights table,
that can be used on both schema insights and statement
details page.

This commit only introduce the table, with the different
types, but still needs the proper cell formating for
each type.

Part of cockroachdb#83783

Release note: None
@maryliag maryliag changed the title ui: new index recommendations table component ui: new insights table component Aug 10, 2022
Copy link
Contributor Author

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @THardy98)


pkg/ui/workspaces/cluster-ui/src/indexRecommendationsTable/indexRecommendationsTable.tsx line 31 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I see on the figma (https://www.figma.com/file/xpnRkP9cLXERVceRC4cec4/22.2_SQL-obsrv_query-performance?node-id=301%3A3805) there could be schema insights unrelated to index recs like Outdated Table Statistics, despite that, are we we okay with keeping this table as "Index Insights" ?

Renamed


pkg/ui/workspaces/cluster-ui/src/indexRecommendationsTable/indexRecommendationsTable.tsx line 73 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I think should be Drop Unused Index, currently that's the only index recommendation we recommend for index drops.

Done

Copy link

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

Copy link

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

@maryliag
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 10, 2022

Build failed (retrying...):

@craig craig bot merged commit 0ad97e4 into cockroachdb:master Aug 10, 2022
@craig
Copy link
Contributor

craig bot commented Aug 10, 2022

Build succeeded:

@maryliag maryliag deleted the insight-component branch August 10, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants