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: Statement diagnostics history - empty state #46921

Merged
merged 1 commit into from Jun 16, 2020

Conversation

vladlos
Copy link
Contributor

@vladlos vladlos commented Apr 2, 2020

Use SortableTable component instead of antd table, this change gives us oportunity to reuse realisation of empty and loading states of table on Statement diagnostics history page.
Minor updates to component props api, to avoid duplicates of window.open calls
Added storybook for SortableTable empty state.

Resolves: #46576

Release note (ui): new design of empty state of Statement diagnostics history page

Screenshot 2020-04-24 at 16 11 20

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vladlos vladlos changed the title ui: Statement Diagnostics History Empty BLOCKED BY #45981 ui: Statement Diagnostics History Empty State Apr 24, 2020
@vladlos vladlos marked this pull request as ready for review April 24, 2020 13:12
@vladlos vladlos requested a review from a team as a code owner April 24, 2020 13:12
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

While I agree that re-using the components is generally the right thing to do, this change modifies the functionality of this page in a big way: previously the links to statement diagnostics would take the you to the statements page.

Additionally the tooltip on a "Ready" diagnostic is now confusing because it tells you to go to the details page but you can't...

Can you make the change without modifying the table itself? Let me know if that's tricky and we can discuss alternatives.

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


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

            description: "Statement diagnostics can help when troubleshooting issues with specific queries. The diagnostic bundle can be activated from individual statement pages and will include EXPLAIN plans, table statistics, and traces.",
            label: "Learn more",
            onClick: () => window.open(statementsTable),

can we change the onClick to be a link href instead? do we ever need to support a special click action?

Copy link
Collaborator

@dhartunian dhartunian 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 @dhartunian and @elkmaster)


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

Previously, dhartunian (David Hartunian) wrote…

can we change the onClick to be a link href instead? do we ever need to support a special click action?

I don't think there's a need to have there be an onClick function option at all.
Can we change the name to buttonHref or something and make it always a string? Then in the Empty component you can wrap the Button in an Anchor. Is that possible?


pkg/ui/src/views/reports/containers/statementDiagnosticsHistory/index.tsx, line 85 at r2 (raw file):

class StatementDiagnosticsHistoryView extends React.Component<StatementDiagnosticsHistoryViewProps, StatementDiagnosticsHistoryViewState> {
  columns: ColumnDescriptor<IStatementDiagnosticsReport>[] = [

Can we set the mouse icon to "pointer" on the column headers?


pkg/ui/src/views/reports/containers/statementDiagnosticsHistory/index.tsx, line 116 at r2 (raw file):

    {
      title: "Status",
      sort: record => record.completed.toString(),

Clicking on status sort breaks the page


pkg/ui/src/views/reports/containers/statementDiagnosticsHistory/index.tsx, line 164 at r2 (raw file):

      sortSetting: {
        sortKey: 0,
        ascending: true,

The default sort order is descending. we'd like to see the newest diagnostics at the top by default.


pkg/ui/src/views/reports/containers/statementDiagnosticsHistory/index.tsx, line 230 at r2 (raw file):

            description: "Statement diagnostics can help when troubleshooting issues with specific queries. The diagnostic bundle can be activated from individual statement pages and will include EXPLAIN plans, table statistics, and traces.",
            label: "Learn more",
            onClick: statementsTable,

This link should be to statementDiagnostics, not statementsTable.

@blathers-crl
Copy link

blathers-crl bot commented Apr 28, 2020

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

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

@vladlos
Copy link
Contributor Author

vladlos commented Apr 28, 2020

Clicking on status sort breaks the page

couldn't recreate this on, my dataset. provided fix by guess, should be retested.

onClick function option is needed, but also added buttonHref to avoid inconvenience in naming. Added ts rule to accept only one of that props at a time.

@blathers-crl
Copy link

blathers-crl bot commented Apr 28, 2020

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

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

@vladlos vladlos force-pushed the feature/diagnostics-empty branch 2 times, most recently from a7a9253 to 13af0ad Compare May 22, 2020 14:23
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

@elkmaster I expected to see the empty state at https://localhost:3000/#/reports/statements/diagnosticshistory when running this. Instead I still see:

Screen Shot 2020-05-26 at 6 42 44 PM

Let me know if I'm missing something.
Please add a Storybook story rendering the empty table with the empty state showing.

Additionally, please describe in the commit message, PR, and Release Notes which pages will have modified behavior and in which cases we expect the empty state to show. Clarify that the empty state provides documentation to customers.

@vladlos vladlos force-pushed the feature/diagnostics-empty branch 2 times, most recently from c4daf23 to c8092f7 Compare May 28, 2020 13:30
@vladlos vladlos changed the title ui: Statement Diagnostics History Empty State ui: Statement diagnostics history - empty state May 28, 2020
@vladlos
Copy link
Contributor Author

vladlos commented May 28, 2020

@dhartunian, sorry just a typo. Should be fixed now. Updated description and added storybook for empty table. In this visible updates will be only on Statement diagnostics history page.

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

@elkmaster LGTM. Thanks for making the latest round of changes. Please rebase and resolve conflicts, then merge with bors.

Use `SortableTable` component instead of antd table, this change gives us oportunity to reuse realisation of empty and loading states of table on Statement diagnostics history page.
Minor updates to <Empty/> component props api, to avoid duplicates of window.open calls
Added storybook for SortableTable empty state.

Resolves: cockroachdb#46576

Release note (ui): new design of empty state of Statement diagnostics history page
@vladlos
Copy link
Contributor Author

vladlos commented Jun 16, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 16, 2020

Build succeeded

@craig craig bot merged commit bc4f495 into cockroachdb:master Jun 16, 2020
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.

ui: statement diagnostics history empty state
3 participants