From 5637518fe148074df596a5c53d13b2ced077d90a Mon Sep 17 00:00:00 2001 From: Jess Telford Date: Tue, 7 May 2019 18:07:05 +1000 Subject: [PATCH] Show loading spinner while loading views in List Table (#1085) --- .changeset/f4c4c52c/changes.json | 1 + .changeset/f4c4c52c/changes.md | 1 + .../admin-ui/client/components/ListTable.js | 182 ++++++++++++------ .../{pages/List => components}/NoResults.js | 10 +- packages/admin-ui/client/pages/List/index.js | 103 ++++------ .../cypress/integration/list/admin-ui.js | 16 +- .../relationship-field-re-hydration_spec.js | 6 +- 7 files changed, 192 insertions(+), 127 deletions(-) create mode 100644 .changeset/f4c4c52c/changes.json create mode 100644 .changeset/f4c4c52c/changes.md rename packages/admin-ui/client/{pages/List => components}/NoResults.js (84%) diff --git a/.changeset/f4c4c52c/changes.json b/.changeset/f4c4c52c/changes.json new file mode 100644 index 00000000000..8be2d032adf --- /dev/null +++ b/.changeset/f4c4c52c/changes.json @@ -0,0 +1 @@ +{ "releases": [{ "name": "@keystone-alpha/admin-ui", "type": "patch" }], "dependents": [] } diff --git a/.changeset/f4c4c52c/changes.md b/.changeset/f4c4c52c/changes.md new file mode 100644 index 00000000000..036a882f17a --- /dev/null +++ b/.changeset/f4c4c52c/changes.md @@ -0,0 +1 @@ +- Show loading spinner while loading views in List Table \ No newline at end of file diff --git a/packages/admin-ui/client/components/ListTable.js b/packages/admin-ui/client/components/ListTable.js index e152585f709..d8fbeb618b2 100644 --- a/packages/admin-ui/client/components/ListTable.js +++ b/packages/admin-ui/client/components/ListTable.js @@ -1,9 +1,10 @@ /** @jsx jsx */ import { jsx } from '@emotion/core'; -import React, { Component } from 'react'; +import React, { Component, Suspense, Fragment } from 'react'; import styled from '@emotion/styled'; import { Link } from 'react-router-dom'; +import { captureSuspensePromises, noop } from '@keystone-alpha/utils'; import { DiffIcon, KebabHorizontalIcon, LinkIcon, ShieldIcon, TrashcanIcon } from '@arch-ui/icons'; import { colors, gridSize } from '@arch-ui/theme'; import { alpha } from '@arch-ui/color-utils'; @@ -15,6 +16,10 @@ import { Card } from '@arch-ui/card'; import DeleteItemModal from './DeleteItemModal'; import { copyToClipboard } from '../util'; import { useListSort } from '../pages/List/dataHooks'; +import PageLoading from './PageLoading'; +import { NoResults } from './NoResults'; + +const Render = ({ children }) => children(); // Styled Components const Table = styled('table')({ @@ -214,12 +219,6 @@ class ListRow extends Component { {fields.map(field => { const { path } = field; - const isLoading = !item.hasOwnProperty(path); - - if (isLoading) { - return ; // TODO: Better loading state? - } - if (itemErrors[path] instanceof Error && itemErrors[path].name === 'AccessDeniedError') { return ( @@ -292,6 +291,12 @@ class ListRow extends Component { } } +const SingleCell = ({ columns, children }) => ( + + {children} + +); + export default function ListTable(props) { const { adminPath, @@ -304,68 +309,135 @@ export default function ListTable(props) { onChange, onSelectChange, selectedItems, + currentPage, + filters, + search, } = props; const [sortBy, onSortChange] = useListSort(list.key); const handleSelectAll = () => { - const allSelected = items.length === selectedItems.length; + const allSelected = items && items.length === selectedItems.length; const value = allSelected ? [] : items.map(i => i.id); onSelectChange(value); }; const cypressId = 'ks-list-table'; - return ( - - - - - {fields.map(f => ( - - ))} - - - - - -
- -
- - {fields.map(field => ( - ( + + + + {fields.map(f => ( + + ))} + + + + + +
+ - ))} - {columnControl} -
- - - {items.map((item, itemIndex) => ( - `${adminPath}/${path}/${id}`} - list={list} - onDelete={onChange} - onSelectChange={onSelectChange} + + + {fields.map(field => ( + ))} - + {columnControl} + + + {children} + + ); + + return ( + +
+ + + + + + } + > + + {() => { + // Now that the network request for data has been triggered, we + // try to initialise the fields. They are Suspense capable, so may + // throw Promises which will be caught by the above + captureSuspensePromises( + fields + .filter(field => field.path !== '_label_') + .map(field => () => field.initCellView()) + ); + + // NOTE: We don't check for isLoading here because we want to + // avoid showing the component when we already + // have (possibly stale) data to show. + // Instead, we show the loader when there's _no data at all_. + if (!items) { + return ( + + + + + + ); + } + + if (!items.length) { + return ( + + + + + + ); + } + + return ( + + {items.map((item, itemIndex) => ( + `${adminPath}/${path}/${id}`} + list={list} + onDelete={onChange} + onSelectChange={onSelectChange} + /> + ))} + + ); + }} + +
); diff --git a/packages/admin-ui/client/pages/List/NoResults.js b/packages/admin-ui/client/components/NoResults.js similarity index 84% rename from packages/admin-ui/client/pages/List/NoResults.js rename to packages/admin-ui/client/components/NoResults.js index 75e8b4e1437..e8e265f81e0 100644 --- a/packages/admin-ui/client/pages/List/NoResults.js +++ b/packages/admin-ui/client/components/NoResults.js @@ -6,7 +6,7 @@ import { Button } from '@arch-ui/button'; import { InfoIcon } from '@arch-ui/icons'; import { colors } from '@arch-ui/theme'; -import { useListPagination } from './dataHooks'; +import { useListPagination } from '../pages/List/dataHooks'; const NoResultsWrapper = ({ children, ...props }) => (
(
); -export const NoResults = ({ currentPage, filters, itemCount, list, search }) => { +export const NoResults = ({ currentPage, filters, list, search }) => { const { onChange } = useListPagination(list.key); const onResetPage = () => onChange(1); @@ -64,9 +64,5 @@ export const NoResults = ({ currentPage, filters, itemCount, list, search }) => ); } - if (itemCount === 0) { - return No {list.plural.toLowerCase()} to display yet...; - } - - return null; + return No {list.plural.toLowerCase()} to display yet...; }; diff --git a/packages/admin-ui/client/pages/List/index.js b/packages/admin-ui/client/pages/List/index.js index 51041048954..e5f1a7372b3 100644 --- a/packages/admin-ui/client/pages/List/index.js +++ b/packages/admin-ui/client/pages/List/index.js @@ -1,7 +1,7 @@ /** @jsx jsx */ import { jsx } from '@emotion/core'; -import { Fragment, Suspense, useEffect, useRef, useState } from 'react'; +import { Fragment, useEffect, useRef, useState } from 'react'; import { Query } from 'react-apollo'; import { IconButton } from '@arch-ui/button'; @@ -18,7 +18,6 @@ import CreateItemModal from '../../components/CreateItemModal'; import DocTitle from '../../components/DocTitle'; import ListTable from '../../components/ListTable'; import PageError from '../../components/PageError'; -import PageLoading from '../../components/PageLoading'; import { DisclosureArrow } from '../../components/Popout'; import { deconstructErrorsToDataShape } from '../../util'; @@ -28,7 +27,6 @@ import SortPopout from './SortSelect'; import Pagination, { getPaginationLabel } from './Pagination'; import Search from './Search'; import Management, { ManageToolbar } from './Management'; -import { NoResults } from './NoResults'; import { useListFilter, useListSelect, useListSort, useListUrlState } from './dataHooks'; const HeaderInset = props => ( @@ -204,66 +202,52 @@ function ListLayout(props: LayoutProps) { /> - {items ? ( - }> - {items.length ? ( - ( - - {ref => ( - - )} - - )} - /> - } - fields={fields} - handleSortChange={handleSortChange} - isFullWidth - items={items} - itemsErrors={itemErrors} - list={list} - onChange={onDeleteItem} - onSelectChange={onSelectChange} - selectedItems={selectedItems} - sortBy={sortBy} - /> - ) : ( - - )} - - ) : ( - - )} + ( + + {ref => ( + + )} + + )} + /> + } + fields={fields} + handleSortChange={handleSortChange} + isFullWidth + items={items} + itemsErrors={itemErrors} + list={list} + onChange={onDeleteItem} + onSelectChange={onSelectChange} + selectedItems={selectedItems} + sortBy={sortBy} + currentPage={currentPage} + filters={filters} + search={search} + /> ); } function List(props: Props) { - const { adminMeta, list, query, routeProps } = props; - const { urlState } = useListUrlState(list.key); + const { list, query, routeProps } = props; // get item data let items; @@ -296,11 +280,6 @@ function List(props: Props) { } }, []); - // TODO: put this in some effect to limit calls - // we want to preload the Field components - // so that we don't have a waterfall after the data loads - adminMeta.preloadViews(urlState.fields.map(({ views }) => views && views.Cell).filter(x => x)); - // Error // ------------------------------ // Only show error page if there is no data diff --git a/test-projects/access-control/cypress/integration/list/admin-ui.js b/test-projects/access-control/cypress/integration/list/admin-ui.js index ff0e1fa9baf..7d10e597f06 100644 --- a/test-projects/access-control/cypress/integration/list/admin-ui.js +++ b/test-projects/access-control/cypress/integration/list/admin-ui.js @@ -363,11 +363,17 @@ describe('Access Control Lists > Admin UI', () => { cy.visit(`admin/${slug}`); + cy.get('#ks-list-table > [data-test-table-loaded=true]'); + // The first label inside thead wraps a visibly-hidden checkbox which // cypress can't find cy.get('#ks-list-table > thead label') .first() - .click(); + // It's there, it's visible in the recordings, but Cypress + // _sometimes_ refuses to click it. + // See an image of it incorrectly failing here: + // https://17679-128193054-gh.circle-artifacts.com/0/tmp/screenshots/list/admin-ui.js/Access%20Control%20Lists%20%20Admin%20UI%20--%20updating%20--%20static%20--%20does%20not%20show%20update%20option%20when%20not%20updatable%20%28list%20view%29%20%7Bcreatefalse%2Creadtrue%2Cupdatefalse%2Cdeletefalse%7D%20%28failed%29.png + .click({ force: true }); cy.get('button[data-test-name="update"]').should('exist'); }); @@ -396,11 +402,17 @@ describe('Access Control Lists > Admin UI', () => { cy.visit(`admin/${slug}`); + cy.get('#ks-list-table > [data-test-table-loaded=true]'); + // The first label inside thead wraps a visibly-hidden checkbox which // cypress can't find cy.get('#ks-list-table > thead label') .first() - .click(); + // It's there, it's visible in the recordings, but Cypress + // _sometimes_ refuses to click it. + // See an image of it incorrectly failing here: + // https://17679-128193054-gh.circle-artifacts.com/0/tmp/screenshots/list/admin-ui.js/Access%20Control%20Lists%20%20Admin%20UI%20--%20updating%20--%20static%20--%20does%20not%20show%20update%20option%20when%20not%20updatable%20%28list%20view%29%20%7Bcreatefalse%2Creadtrue%2Cupdatefalse%2Cdeletefalse%7D%20%28failed%29.png + .click({ force: true }); cy.get('button[data-test-name="update"]').should('not.exist'); }); diff --git a/test-projects/basic/cypress/integration/relationship-field-re-hydration_spec.js b/test-projects/basic/cypress/integration/relationship-field-re-hydration_spec.js index cc7c72453ff..89a1c2b1345 100644 --- a/test-projects/basic/cypress/integration/relationship-field-re-hydration_spec.js +++ b/test-projects/basic/cypress/integration/relationship-field-re-hydration_spec.js @@ -11,7 +11,11 @@ describe('Testing re-hydration', () => { // And now select and click the actually rendered element. cy.get('#react-select-ks-input-categories div[aria-hidden="true"]') .first() - .click(); + // It's there, it's visible in the recordings, but Cypress _sometimes_ + // refuses to click it. + // See an image of it incorrectly failing here: + // https://17680-128193054-gh.circle-artifacts.com/0/tmp/screenshots/relationship-field-re-hydration_spec.js/Testing%20re-hydration%20--%20Our%20new%20category%20should%20appear%20after%20we%20add%20it%20%28failed%29.png + .click({ force: true }); } cy.visit('/admin/posts');