ui: Empty state

[vladlos]

tables empty state redesign, created reusable component for no results state

Resolves: #45264

Release justification: bug fixes and low-risk updates to new functionality

Release note (ui): new design of empty state on Jobs, Diagnostics, Statements and StatementsDetails pages

[cockroach-teamcity]

This change is 

[dhartunian]

ui: Empty state

This is not descriptive enough. Please keep it to 50 chars but mention what was done.

I really like that this change moves a component from src/views/app/... into the upper level components directory. 👏

The commit message needs more detail.
I'd like to see:

• what the Empty component is and where we should use it. Is it only for tables, for instance, or can I use it anywhere?
• a list of components that the Empty component is now being used in
• discussion of new props being added to components that facilitate use of Empty component

Reviewed 15 of 17 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @elkmaster)

---

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

export const adminUIAccess = docsURL("admin-ui-overview.html#admin-ui-access");
export const statementDiagnostics = docsURLNoVersion("admin-ui-statements-page.html#diagnostics");
export const databaseTable = docsURLNoVersion("admin-ui-databases-page.html");

Why are these links noVersion? I would assume we would link to the versioned one.

---

pkg/ui/src/views/databases/containers/tableDetails/index.tsx, line 34 at r1 (raw file):

import { databaseDetails } from "../databaseSummary";
import { Button, BackIcon } from "oss/src/components/button";
import { databaseTable } from "oss/src/util/docs";

We need to stop using oss/ as a prefix for imports. Import should always come from /src. The oss or ccl prefix is added by webpack. This is to allow ccl directory to override. Can you change just the ones in this PR for now? I will file a ticket to fix the rest.

---

pkg/ui/src/views/databases/containers/tableDetails/index.tsx, line 103 at r1 (raw file):

      return statements.filter((statement: AggregateStatistics) => statement.label.indexOf(title) !== -1);
    }
    return [];

thanks for this, we should explore turning on strict null checks 🤔

---

pkg/ui/src/views/databases/containers/tableDetails/index.tsx, line 166 at r1 (raw file):

              </TabPane>
              <TabPane tab="Statements" key="2">
                <StatementsSortedTable

Did you explore a different style of API where the Empty component wraps an existing component? We could have:

<Empty showWhen={statementsData.length === 0} ... >
  <StatementsSortedTable ... />
</Empty>

Then you can avoid having to implement the if (empty) { return <Empty... inside every other component.

What do you think?

---

pkg/ui/src/views/databases/containers/tableDetails/index.tsx, line 179 at r1 (raw file):

              </TabPane>
              <TabPane tab="Grants" key="3">
                <GrantsSortedTable

Why doesn't this have empty prop?

---

pkg/ui/src/views/jobs/jobTable.tsx, line 161 at r1 (raw file):

          columns={jobsTableColumns}
          renderNoResult={this.noJobResult()}
          empty={_.isEmpty(jobs) && !this.props.isUsedFilter}

Why do we need empty and renderNoResult? Couldn't we do both with just the empty functionality?

---

pkg/ui/src/views/shared/components/sortabletable/index.tsx, line 17 at r1 (raw file):

import { DrawerComponent } from "../drawer";
import "./sortabletable.styl";
import Empty, { IEmptyProps } from "oss/src/components/empty/empty";

this import looks weird, no? can we just import from /components/empty?

[dhartunian]

One last comment: the change is failing the linter because of a missing Release justification.

[vladlos]

Did you explore a different style of API where the Empty component wraps an existing component? We could have…

In my opinion "wrapping" components should be used only when there is no other way, for example for real wrappers, like layout or anything that adds something before and\or after {children}. In current case it will just add unnecessary complexity.

Why doesn’t this have empty prop?

Some screens have empty prop implementations as separated prs.

Why do we need empty and renderNoResult? Couldn’t we do both with just the empty functionality?

No result state is only for tables with filters, when we have data, but nothing is matching filter criteria. Maybe make sense to create another ticket for moving it out in component too.

[dhartunian]

@elkmaster can you update the release note to explain exactly which pages on the site are changing. I assume it's just the 4 mentioned in the issue.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @elkmaster)

[vladlos]

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on faee5c4a.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

[dhartunian]

Reviewed 11 of 11 files at r2, 8 of 8 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @elkmaster)

---

pkg/ui/src/components/empty/empty.tsx, line 36 at r4 (raw file):

  backgroundImage,
}: IEmptyProps) => (
  <div className="cl-empty-view" style={{ backgroundImage: `url(${backgroundImage})` }}>

Is this currently overridden by any user of this component? Why not have it set in the CSS?

[vladlos]

Is this currently overridden by any user of this component? Why not have it set in the CSS?

Now it not used but I think I seen some designs with different images, so for easier customisation in future it was done with inline styles.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 65a57212.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 79265313.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 80f32009.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[dhartunian]

@elkmaster this looks good! thanks for the edits.

It looks like you have an edit to yarn-vendor that should not be there. Can you remove that, rebase and resolve conflicts so we can merge?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @elkmaster)

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 3e1efe4c.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on f2790ec8.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 40d23e70.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[dhartunian]

thanks for the update

Reviewed 15 of 15 files at r5, 7 of 7 files at r6, 2 of 5 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @elkmaster)

[dhartunian]

bors r+

[craig]

Merge conflict (retrying...)

[dhartunian]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)