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: Sortable loading state #46857

Merged
merged 3 commits into from
Apr 21, 2020

Conversation

vladlos
Copy link
Contributor

@vladlos vladlos commented Apr 1, 2020

Added global loading state to sort tables
Added loading state to database tables

loading-database

Resolves: #46568

Release justification: low risk, high benefit changes to existing functionality

Release note (ui): database loading state design updates

@vladlos vladlos requested a review from a team April 1, 2020 15:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vladlos vladlos force-pushed the feature/loading-database-state branch 2 times, most recently from 5afdfc9 to 280fa5e Compare April 2, 2020 10:31
@blathers-crl
Copy link

blathers-crl bot commented Apr 8, 2020

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

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

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


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

class DatabaseTableListSortedTable extends SortedTable<TableInfo> {}

class DatabaseTableListEmpty extends React.Component {

it looks like the existing message of "This database has no tables" is removed with this PR. Can we keep that in for databases that in fact, have no tables.


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

                  sortSetting={sortSetting}
                  onChangeSortSetting={(setting) => this.props.setSort(setting)}
                  loading={!tableInfos}

Did you happen to see if we could access the inFlight property on the cachedData objects? Will this show as "loading" if the response is empty?

Added global loading state to sort tables
Added loading state to database tables

Resolves: cockroachdb#46568

Release justification: low risk, high benefit changes to existing functionality

Release note (ui): database loading state design updates
returned empty state for database tables

used `isFlight` for Database table loading state

Release note (ui): database loading state design updates
@vladlos vladlos force-pushed the feature/loading-database-state branch from 280fa5e to 664ea99 Compare April 14, 2020 09:32
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.

Reviewed 4 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @elkmaster)


pkg/ui/src/views/databases/containers/databaseSummary/index.tsx, line 120 at r3 (raw file):

// export const isTableFlight = (state: AdminUIState, dbName: string) {

// }

Remove commented code please :)


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

          <div className="table__loading">
            <Spin className="table__loading--spin" indicator={<Icon component={SpinIcon} spin />} />
            <span className="table__loading--label">Loading tables...</span>

This text is specific to the database "Tables". Can we extract the component up to the database tables file? Then it can live next to the "no results" component which I think will help readability.

Removed commented code
Moved specific table loading text to the upper level to be able to reuse it

Release note (ui): database loading state design updates
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.

:lgtm: thanks for this last round of changes.

Reviewed 2 of 2 files at r3, 4 of 4 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @elkmaster)

@dhartunian
Copy link
Collaborator

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 21, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Apr 21, 2020

Build succeeded

@craig craig bot merged commit 998abbe into cockroachdb:master Apr 21, 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: Add loading state to tables on database page
3 participants