ui: rebuild the databases pages #68390

matthewtodd · 2021-08-03T18:53:21Z

Previously, the databases pages featured an older, outdated design. This
change aligns their UX with that of the other modern pages in the DB
console, statements, transactions, and sessions.

This is a first pass at the job. We will return to layer on more
features, including search, and more data, including node & region
information.

Addresses #65737.

Before	After


	(This overview of database grants no longer exists.)
(This rolled-up view of table grants did not previously exist.)

Release note (ui change): The databases pages in the DB console were
updated to bring them into alignment with our modern UX.

cockroach-teamcity · 2021-08-03T18:53:27Z

This change is

maryliag

Reviewed 53 of 64 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: file name databaseDetailsPage

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 42 at r1 (raw file):

const sortableTableCx = classNames.bind(sortableTableStyles);

export interface DatabasePageDataTableDetails {

nit: add comments for all data interfaces

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 141 at r1 (raw file):

  }

  private changeView(view: ViewOption) {

nit: maybe a comment for now (or think of a clear name)

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 145 at r1 (raw file):

  }

  private columns(): ColumnDescriptor<DatabasePageDataTable>[] {

nit: separate between table and grant

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 274 at r1 (raw file):

  }

  private viewSelectionTitle(): string {

nit: inline this function

pkg/ui/cluster-ui/src/databasePage/databasePage.module.scss, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: file name databaseDetailsPage

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 37 at r1 (raw file):

const sortableTableCx = classNames.bind(sortableTableStyles);

export interface DatabasesPageDataMissingTable {

nit: add comments for interfaces

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 16 at r1 (raw file):

import _ from "lodash";

import { AlignedIcon } from "src/icon/alignedIcon";

nit: group with icon import

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 35 at r1 (raw file):

}

export interface DatabaseTablePageDataDetails {

nit: add comments for interfaces

pkg/ui/cluster-ui/src/icon/alignedIcon.tsx, line 17 at r1 (raw file):

const cx = classNames.bind(styles);

export interface AlignedIconProps {

check titleWithIcon component

pkg/ui/cluster-ui/src/icon/icon.tsx, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

we might want to create a task for refactor current usage of icons, so they're all in this same place

pkg/ui/cluster-ui/src/storybook/fixtures/randomName.ts, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

check if these files are being added to the build

pkg/ui/src/util/fakeApi.ts, line 11 at r1 (raw file):

// licenses/APL.txt.

import * as $protobuf from "protobufjs";

nit: add comment about usage of this file (for testing)
add examples

pkg/ui/src/views/databases/databasePage/redux.spec.ts, line 196 at r1 (raw file):

    await driver.refreshDatabaseDetails();
    await driver.refreshTableDetails("foo");

create a test for checking two tables with different values

pkg/ui/src/views/databases/databasePage/redux.spec.ts, line 216 at r1 (raw file):

    fakeApi.stubTableDetails("things", "foo", {
      grants: [
        { user: "bloblaw", privileges: ["ALL"] },

nit: add another role to test the alphabetical order

pkg/ui/src/views/databases/databasesPage/redux.ts, line 27 at r1 (raw file):

const { DatabaseDetailsRequest, TableStatsRequest } = cockroach.server.serverpb;

export const mapStateToProps = createSelector(

consider extracting this outside of this redux to a place that can be used by crdb and cloud db (and serverless )

pkg/ui/src/views/databases/databasesPage/redux.ts, line 55 at r1 (raw file):

        // all of their individual stats.

        const possiblyMissingTables = stats

consider a different visual for the user when something is missing. Discuss with Anne possible solutions. We want the user to know that some data might be missing.

matthewtodd

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

pkg/ui/cluster-ui/src/icon/icon.tsx, line 1 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

we might want to create a task for refactor current usage of icons, so they're all in this same place

Done, #68501

pkg/ui/cluster-ui/src/storybook/fixtures/randomName.ts, line 1 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

check if these files are being added to the build

All clear!

$ grep randomName pkg/ui/cluster-ui/dist/main.js
$ grep randomRole pkg/ui/cluster-ui/dist/main.js
$ grep randomTablePrivilege pkg/ui/cluster-ui/dist/main.js
$

pkg/ui/src/views/databases/databasesPage/redux.ts, line 27 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

consider extracting this outside of this redux to a place that can be used by crdb and cloud db (and serverless )

Good idea, I think it'll make sense to do this extraction as we're starting to support cloud / serverless.

pkg/ui/src/views/databases/databasesPage/redux.ts, line 55 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

consider a different visual for the user when something is missing. Discuss with Anne possible solutions. We want the user to know that some data might be missing.

Good idea, I've captured a note to follow up with Anne when she gets back. It is unclear to me how common this case is and how much we need to design for it; just that's it's possible. In the meantime, per our brief conversation in today's standup, we'll look into it as part of a follow-on task, not for this PR.

pkg/ui/cluster-ui/src/icon/alignedIcon.tsx, line 17 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

check titleWithIcon component

Done, merged with the Icon component!

pkg/ui/src/views/databases/databasePage/redux.spec.ts, line 196 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

create a test for checking two tables with different values

Done.

Azhng

Reviewed 10 of 64 files at r1, 12 of 19 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 83 at r2 (raw file):

// those properties for the table directly.
export interface DatabasesPageDataMissingTable {
  loading: boolean;

why there is no need for loaded field here?

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

    }

    _.forEach(this.props.databases, database => {

Do we have a preference on when to use lodash vs the builtin functions of js array ?

pkg/ui/src/redux/analytics.ts, line 58 at r2 (raw file):

  },
  {
    match: new RegExp("/database/.+"),

Do matchers here run in sequential orders as they are defined? It seems like /database/.+ would also match /database/.+/grants

pkg/ui/src/views/databases/databaseTablePage/redux.spec.ts, line 107 at r2 (raw file):

    driver = new TestDriver(
      createAdminUIStore(createMemoryHistory()),
      "things",

super nit: got thrown off a bit reading through this, perhaps s/thing/fakeDbName/ and s/foo/fakeTableName/

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 34 at r2 (raw file):

} = cockroach.server.serverpb;

function normalizeRoles(raw: string[]): string[] {

nit: perhaps normalizeRolesOrdering ?

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 82 at r2 (raw file):

  ): DatabaseDetailsPageData => {
    return {
      loading: !!databaseDetails[database]?.inFlight,

what is this !! operator?

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 86 at r2 (raw file):

      name: database,
      tables: _.map(databaseDetails[database]?.data?.table_names, (table) => {
        const details = tableDetails[generateTableID(database, table)];

nit: save the tableID to avoid repeatedly calling generateTableID ?

matthewtodd

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

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 83 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

why there is no need for loaded field here?

Good call, this one breaks the pattern. I'll add a comment.

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Do we have a preference on when to use lodash vs the builtin functions of js array ?

Oh, I didn't realize most of what I wanted to do was already on Array -- it's been a while for me! I think it makes sense to lean less on an external dependency where we can, so I'll be happy to remove lodash where possible.

pkg/ui/src/redux/analytics.ts, line 58 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Do matchers here run in sequential orders as they are defined? It seems like /database/.+ would also match /database/.+/grants

Yes, the redaction function runs through these until it finds the first match; that's why the more specific matches come first in the list.

pkg/ui/src/views/databases/databaseTablePage/redux.spec.ts, line 107 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

super nit: got thrown off a bit reading through this, perhaps s/thing/fakeDbName/ and s/foo/fakeTableName/

Done.

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 34 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: perhaps normalizeRolesOrdering ?

This function (and normalizePrivileges, below) does a little more than just ordering, so I kind of like normalize as it stands. Did it seem unclear?

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 82 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

what is this !! operator?

Ooh! It's a double-not -- you can pronounce it "bang bang!" -- good for turning "truthy" and "falsy" (false, null, undefined) into true and false. I'm using it here because databaseDetails[database] might be undefined, so inFlight might also be, and having a reliable boolean in the props made me and typescript a little happier. :-) Is there a more modern way to do this I don't know about?

Azhng

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Oh, I didn't realize most of what I wanted to do was already on Array -- it's been a while for me! I think it makes sense to lean less on an external dependency where we can, so I'll be happy to remove lodash where possible.

👍

pkg/ui/src/redux/analytics.ts, line 58 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Yes, the redaction function runs through these until it finds the first match; that's why the more specific matches come first in the list.

👍

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 34 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

This function (and normalizePrivileges, below) does a little more than just ordering, so I kind of like normalize as it stands. Did it seem unclear?

👍

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 82 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Ooh! It's a double-not -- you can pronounce it "bang bang!" -- good for turning "truthy" and "falsy" (false, null, undefined) into true and false. I'm using it here because databaseDetails[database] might be undefined, so inFlight might also be, and having a reliable boolean in the props made me and typescript a little happier. :-) Is there a more modern way to do this I don't know about?

Ah I see. I guess this is the price we pay for dynamic typing lol.

maryliag

Reviewed 1 of 19 files at r2, 5 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 215 at r3 (raw file):

                    <SummaryCardItem
                      label="Indexes"
                      value={_.join(this.props.details.indexNames, ", ")}

nit: similar message as the others, you can join without the use of lodash

pkg/ui/cluster-ui/src/icon/icon.module.scss, line 9 at r3 (raw file):

// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

did you made a check on all places using icon to make sure none of them misaligned?

pkg/ui/src/views/databases/databasesPage/redux.ts, line 36 at r3 (raw file):

      loading: databases.inFlight,
      loaded: databases.valid,
      databases: _.map(databases.data?.databases, (database) => {

nit: use map directly on the array (other example on this same file, same for .each)

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 156 at r3 (raw file):

    }

    _.forEach(this.props.tables, table => {

nit: no need for lodash here either, all the forEach can be directly on the array

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 300 at r3 (raw file):

      {
        title: "Roles",
        cell: table => _.join(table.details.roles, ", "),

nit for all joins: table.details.roles.join(", ")

Previously, the databases pages featured an older, outdated design. This change aligns their UX with that of the other modern pages in the DB console, statements, transactions, and sessions. This is a first pass at the job. We will return to layer on more features, including search, and more data, including node & region information. Addresses #65737. Release note (ui change): The databases pages in the DB console were updated to bring them into alignment with our modern UX.

matthewtodd

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

👍

#68820

pkg/ui/cluster-ui/src/icon/icon.module.scss, line 9 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

did you made a check on all places using icon to make sure none of them misaligned?

This was a new file that only the new components used, and I checked them all. I've since unrolled it because its strategy of using data-uris for svgs prevented us from using css to size and color the icons.

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 156 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: no need for lodash here either, all the forEach can be directly on the array

Agreed, pushed to #68820 since there are some semantic differences between _.forEach and Array.forEach when undefined is involved.

matthewtodd

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 215 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: similar message as the others, you can join without the use of lodash

Bumped to #68820

pkg/ui/src/views/databases/databasesPage/redux.ts, line 36 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: use map directly on the array (other example on this same file, same for .each)

Bumped to #68820

matthewtodd

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

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 300 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit for all joins: table.details.roles.join(", ")

Bumped to #68820

maryliag

Great work on this, new page looks awesome!
As long as all comments are addressed (and necessary new issues created):

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

matthewtodd

Thanks, @maryliag!

Follow-on issues are:

I've tagged many of these as quick wins so we / I can find them easily on our next quick win day.

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

matthewtodd · 2021-08-12T20:42:21Z

bors r+

craig · 2021-08-12T21:40:37Z

Build succeeded:

GitHub CI (Cockroach)

maryliag suggested changes Aug 4, 2021

View reviewed changes

matthewtodd commented Aug 5, 2021

View reviewed changes

matthewtodd requested a review from a team August 5, 2021 21:05

Azhng reviewed Aug 6, 2021

View reviewed changes

matthewtodd commented Aug 6, 2021

View reviewed changes

Azhng reviewed Aug 9, 2021

View reviewed changes

maryliag reviewed Aug 11, 2021

View reviewed changes

matthewtodd mentioned this pull request Aug 12, 2021

ui: replace lodash with vanilla Array methods where possible in the databases pages #68820

Closed

matthewtodd commented Aug 12, 2021

View reviewed changes

maryliag approved these changes Aug 12, 2021

View reviewed changes

This was referenced Aug 12, 2021

ui: add filtering to the databases pages #68826

Closed

ui: add regions / nodes to the databases pages #68828

Closed

ui: database details page in "grants" mode should link to database table page in "grants" mode #68829

Closed

matthewtodd commented Aug 12, 2021

View reviewed changes

craig bot merged commit ab15e5f into cockroachdb:master Aug 12, 2021

matthewtodd deleted the 65737-databases-main-page branch August 23, 2021 19:13

jseldess mentioned this pull request Sep 8, 2021

ui: rebuild the databases pages cockroachdb/docs#11563

Closed

xinhaoz mentioned this pull request Oct 26, 2021

Zone configurations should be removed from the Table overview page #65760

Closed

exalate-issue-sync bot mentioned this pull request Jun 8, 2022

ui: rebuild the databases pages cockroachdb/docs#14152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: rebuild the databases pages #68390

ui: rebuild the databases pages #68390

matthewtodd commented Aug 3, 2021 •

edited

Loading

cockroach-teamcity commented Aug 3, 2021

maryliag left a comment

matthewtodd left a comment

Azhng left a comment

matthewtodd left a comment

Azhng left a comment

maryliag left a comment

matthewtodd left a comment

matthewtodd left a comment

matthewtodd left a comment

maryliag left a comment

matthewtodd left a comment •

edited

Loading

matthewtodd commented Aug 12, 2021

craig bot commented Aug 12, 2021

ui: rebuild the databases pages #68390

ui: rebuild the databases pages #68390

Conversation

matthewtodd commented Aug 3, 2021 • edited Loading

cockroach-teamcity commented Aug 3, 2021

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment • edited Loading

Choose a reason for hiding this comment

matthewtodd commented Aug 12, 2021

craig bot commented Aug 12, 2021

matthewtodd commented Aug 3, 2021 •

edited

Loading

matthewtodd left a comment •

edited

Loading