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

fix(PaginationTableDemo): add Spinner and empty state to demo #3294

Merged
merged 5 commits into from Nov 21, 2019

Conversation

@nicolethoen
Copy link
Contributor

nicolethoen commented Nov 8, 2019

Fixes #3184

@nicolethoen nicolethoen requested review from boaz0, dlabrecq and mcarrano Nov 8, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 8, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@2415a94). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3294   +/-   ##
=========================================
  Coverage          ?   67.44%           
=========================================
  Files             ?      892           
  Lines             ?    24874           
  Branches          ?     2141           
=========================================
  Hits              ?    16776           
  Misses            ?     7093           
  Partials          ?     1005
Flag Coverage Δ
#misc 95.45% <ø> (?)
#patternfly3 69.29% <ø> (?)
#patternfly4 64.78% <ø> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2415a94...e5e887b. Read the comment docs.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 8, 2019

PatternFly-React preview: https://patternfly-react-pr-3294.surge.sh

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Nov 8, 2019

Looks like we have two PRs for the same issue?
#3232

Copy link
Member

boaz0 left a comment

👍

if (res.length === 0) {
const rows = [{
heightAuto: true,
cells: [
{
props: { colSpan: 8 },
title: (
<Bullseye>
<EmptyState variant={EmptyStateVariant.small}>
<EmptyStateIcon icon={SearchIcon} />
<Title headingLevel="h2" size="lg">
No results found
</Title>
<EmptyStateBody>
No results match the filter criteria. Remove all filters or clear all filters to show results.
</EmptyStateBody>
</EmptyState>
</Bullseye>
)
},
]
}]
Comment on lines 74 to 123

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Nov 11, 2019

Member

How do users see the empty state? Do we need to edit the code to remove rows?

This comment has been minimized.

Copy link
@boaz0

boaz0 Nov 11, 2019

Member

we should probably add a filter to the table and if a filter is set show empty state.

<TableHeader />
<TableBody />
</Table>
)}
{loading && <center><Title size="3xl">Please wait while loading data</Title></center>}
{loading && <center><Spinner size="xl"/></center>}

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Nov 11, 2019

Member

I'm not seeing a spinner. I briefly see the empty state prior to loading the table rows. Is there a way to better see this?

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Nov 11, 2019

@nicolethoen for some reason I don't seem to be able to access the preview link. It says "project not found."

@nicolethoen nicolethoen force-pushed the nicolethoen:empty_state_to_demo branch from cd04844 to c49160e Nov 12, 2019
@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 12, 2019

@dlabrecq @boaz0 I can work to add a way to force view these states.

@nicolethoen nicolethoen force-pushed the nicolethoen:empty_state_to_demo branch from c49160e to 4390c49 Nov 12, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 12, 2019

PatternFly-React preview: https://patternfly-react-pr-3294.surge.sh

Copy link
Member

dlabrecq left a comment

Looks great! Built and ran locally...

Thanks for adding the radio buttons

@nicolethoen nicolethoen force-pushed the nicolethoen:empty_state_to_demo branch from 4390c49 to 2ff4270 Nov 13, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 13, 2019

PatternFly-React preview: https://patternfly-react-pr-3294.surge.sh

Copy link
Member

mcarrano left a comment

@nicolethoen finally was able to preview this. Just a couple of changes to request.

  • Can the header row for the table be shown when the data is being loaded as it does for the empty state?

  • For the empty state, there should be a 'Clear all filters' action associated with the empty state.

See the core demos here for reference: https://www.patternfly.org/v4/documentation/core/demos/table

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 14, 2019

@mcarrano those changes should be no problem! There are no filters on the table though, so the 'Clear All Filters' action would be a dead action, so do we still want it? I removed it intentionally for just that reason. If we don't want a dead action, I should probably change the text under the EmptyStateIcon as well so it doesn't reference filters either. But then I'm not sure the best thing for it to say.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Nov 14, 2019

That's an interesting question @nicolethoen . I think it's important to have the Clear filters action since this message would only really appear as the result of a filter. But you are right that this would not really do anything in the current demo. Would it take much to add a simple filter to this, like just a Select list with some values?

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 14, 2019

@mcarrano we have a filterable table demo... would it need to also use the experimental data toolbar to be implemented correctly? seems out of scope for this demo. The only legitimate reason there would be no data to display is if no data is returned from the API. It'd be more of an error.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Nov 14, 2019

Yes, that's true @nicolethoen . But this is a slightly different empty state pattern in that case. I will try to set up some time to sync with you on what to put here.

@nicolethoen nicolethoen force-pushed the nicolethoen:empty_state_to_demo branch from 2ff4270 to 373c12f Nov 15, 2019
@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 15, 2019

Note: radio buttons were removed - it was decided they were muddying context for the user a little bit. And 'Empty State' was replaced with more of an 'Error state' that is not demonstrated intentionally as part of the demo - but if there is an ever some unknown error retrieving data, it will fail gracefully. Any anyone curious about error handling in this case can look at how it's handled in the code. It was decided this demo is not going to highlight the error state, because it is not an Error State demo, it is a Pagination Table demo.

Copy link
Member

dlabrecq left a comment

Wasn't the issue to add "spinner and empty state" to the demo? It's not obvious that these features exist in this example -- I'm not able to see the spinner, nor the empty state. Is there a way we can solve that?

FYI, I had closed #1433 because this issue was to provide empty state functionality.

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 18, 2019

@dlabrecq I dont think that the Pagination Demo is the place to demonstrate the empty state or error state of a table. It is simply for demonstrating pagination. The other demos don't explicitly demonstrate empty states or error states either.

Before the change, as data was being loaded and paginated, you saw a message that quickly flashed Please wait while loading data before it disappeared. Now you see the spinner instead.

And we've added an error state if no data is retrieved, which should never happen unless there is a connectivity issue - so I added the error state so that any issues that arise will fail gracefully using a Patternfly pattern. So it's not explicitly demonstrable, but is more there as it should if anyone wants to implement Pagination in their table correctly. I think that is following the demo precedent we've established.

These changes were made after talking to @mcarrano and Content strategist Margot

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 18, 2019

I'll also add that the Filterable Table demo does demonstrate an empty state when all options have been filtered out. That seems to be a more relevant place to demonstrate this. @dlabrecq

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Nov 18, 2019

I'm just going by what issue #3184 is asking for. If the functionality is not appropriate for the pagination demo, then perhaps we should not modify this particular demo? Or, leave the issue open?

Regardless, my issue is that I don't even see the spinner.

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 18, 2019

Your machine must be processing the data too fast for you to see it. I almost always see it when I change pagination pages, or when I change the number of items displayed per page. @dlabrecq

@mcarrano would it be appropriate to add a 'View Loading State" checkbox to force the table to stay in a loading state?

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Nov 18, 2019

How about a timeout that loads the table after a second or two? Then, there is no need for a checkbox

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Nov 18, 2019

@nicolethoen @dlabrecq I'm good with either of those approaches. It was the inclusion of the empty state here that was causing me the most confusion. I'm wondering if we should have a demo that shows the various types of empty states in context as there are lots of different reasons to get an empty state. Do you think that would be useful?

@nicolethoen nicolethoen force-pushed the nicolethoen:empty_state_to_demo branch from 5514011 to e5e887b Nov 18, 2019
@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 20, 2019

@dlabrecq I updated the attached issue to reflect the changes made in this PR. And I opened a new issue to document the missing demos. #3330

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Nov 20, 2019

Any other thoughts @dlabrecq or @mcarrano? Can we get this PR in soon :)

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Nov 20, 2019

@nicolethoen It makes sense to me to create these in a separate but related demo. I've left a comment to see if we can get that om the roadmap.

Copy link
Member

mcarrano left a comment

Looks good!

Copy link
Member

dlabrecq left a comment

Thanks for creating a new issue

@dlabrecq dlabrecq merged commit c4cb9f4 into patternfly:master Nov 21, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 21, 2019

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.1.33
  • @patternfly/react-core@3.122.3
  • @patternfly/react-docs@4.16.39
  • @patternfly/react-inline-edit-extension@2.13.4
  • demo-app-ts@3.11.3
  • @patternfly/react-table@2.24.36
  • @patternfly/react-topology@2.11.22
  • @patternfly/react-virtualized-extension@1.3.35

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.