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

Show loading spinner while loading views in List Table #1085

Merged
merged 1 commit into from
May 7, 2019

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented May 3, 2019

NOTE: PR best viewed with whitespace changes disabled

This moves the loading spinner inside the table which allows the user to interact with the sorting / columns display even while data is loading.

It also implements the things in #1077 so the data and the views can load in parallel.

posts-load-views-in-table

@changeset-bot
Copy link

changeset-bot bot commented May 3, 2019

✅ This PR has a changeset ✅
Latest commit: efc7f0e

Click here to learn what changesets are.

@jesstelford jesstelford force-pushed the loader-in-list-table branch 2 times, most recently from b9fe03a to 13b7d4e Compare May 3, 2019 07:45
@jesstelford jesstelford force-pushed the loader-in-list-table branch 2 times, most recently from 447be8f to a79062c Compare May 7, 2019 06:52
@jesstelford
Copy link
Contributor Author

@mitchellhamilton I broke the integration tests with this change so had to rework things slightly (just moved the <Suspense> component up the tree a little bit and added a data- attribute so Cypress can wait for things to be loaded before trying to interact with the table.

Can I get a re-review?

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Does it change the behavior that the user observes? and if so, got some gifs?

const columns = fields.length + 2;

const TableContents = ({ isLoading, children }) =>
console.log({ isLoading }) || (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log({ isLoading }) || (
(

@jesstelford
Copy link
Contributor Author

jesstelford commented May 7, 2019

🤔 Those two test failures were Cypress being silly - both the elements it claims were invisible are in fact visible in the screenshots and videos.

I've added {force: true} to bypass its "visibility" check (which isn't important anyway).

Does it change the behavior that the user observes?

Nope, it's exactly the same 👍

@jesstelford jesstelford merged commit 5637518 into master May 7, 2019
@emmatown emmatown deleted the loader-in-list-table branch May 7, 2019 08:41
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.

2 participants