-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor and revert revert resourcelist emptystate #2569
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
Refactor and revert revert resourcelist emptystate #2569
Conversation
🟢 This pull request modifies 5 files and might impact 1 other files. Details:All files potentially affected (total: 1)📄
|
870363b
to
57f323e
Compare
57f323e
to
abec7af
Compare
d970f85
to
588d37a
Compare
} | ||
|
||
.withinContentContainer { | ||
margin: 0 auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Oversight, top margin should've been removed from the .EmptyState
class in a content context in #1570)
703c101
to
76eb617
Compare
Hi @chloerice. I know this isn't necessarily a breaking change, but I'm wondering if we should target v5 for this. The reason is that this will require consumers to update their code to pass the empty state, which could be a potential blocker from releasing in web. Alternatively, can we find out how many lists will be affected and reach out to the owners? The code looks good but have we considered making |
Hey @dleroux! |
|
||
const showEmptyState = filterControl && !this.itemsExist() && !loading; | ||
const showEmptyStateMarkup = | ||
!loading && showEmptyState && emptyState && !this.itemsExist(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it could lead to confusion to allow the consumer to determine whether they should show the empty state and then internal logic could override that. What do you think about showEmptyState
taking precedence over everything else? Or even if an emptyState
is defined show the empty state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if an emptyState is defined show the empty state?
Ahhh that's what you meant in the other comment! That makes sense, consumers can just use the conditional they'd use to set the value of showEmptyState
as a ternary condition for setting the value of emptyState
. I'll refactor and update the example and prop description 👍
🎩 looks good, just left a comment about the behavior of the prop. |
7d00e86
to
b361473
Compare
…ptystate"" This reverts commit 6ec053f.
5bba223
to
702877c
Compare
702877c
to
e371810
Compare
e371810
to
57a40ac
Compare
filterControl?: React.ReactNode; | ||
/** The markup to display when no resources exist yet. Renders when set and items is empty. */ | ||
emptyState?: React.ReactNode; | ||
/** The markup to display when no results are returned on search or filter of the list. Renders when `filterControl` is set, items are empty, and `emptyState` is not set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could someone have filterControls on a list that has not had resources yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! You can see what that looks like in the playground code--aside from making list views more familiar to new merchants, the reason for moving to in-context empty states is for smooth transition between loading and empty or with-data states 😊. That's why I've got the filterControl set in the empty state example to illustrate that you'd want to still have the filters there but just disable them since there's no list items to take action on.
/** The markup to display when no results are returned on search or filter of the list. Renders when `filterControl` is set, items are empty, and `emptyState` is not set. | ||
* @default EmptySearchResult | ||
*/ | ||
emptySearchState?: React.ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are new props. what will happen to the empty list in web that don't have these props defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're optional props so there's no breaking changes with either. Andre added emptySearchState
because he needed to render custom markup for the no results state instead of EmptySearchResult
for the resource list he's working on. Empty state for the list wasn't handled/supported before this PR, the list would just be empty and have no header. But with the new design language rollout we'll be changing empty states for list views from having the large EmptyState
illustration to having the empty state be in the context of the list so that it's more familiar to merchants once the list has resources.
Just top hatted in web and It looks like it working fine. The logic is getting a little complex, I wonder if explicitly having |
Reverts the revert of #2160.
The initial approach to knowing when to render the
emptyState
when provided used an existing prop,hasMoreItems
, which conflicted with its use by consumers likeweb
to show empty search result markup. That prop is also used internally to determine whether or not to show a "Load more items" link-like button in the list.Initially this PR was adding a new boolean prop,
showEmptyState
so consumers could explicitly tell theResourceList
whether or not to render theemptyState
when provided. After discussing with @dleroux, we've decided thatemptyState
will render when set if items are empty*.*
emptySearchState
is a new prop recently added that complicates this a bit, as no results state is handled internally in spite of filters being a separate component. We have no idea when the list is being filtered/queried and since assumptions were already being made about no results existing whenitems
is empty andloading
isfalse
,emptyState
has to take precedence in that internal conditional in order to delineate between empty and no results states.WHY are these changes introduced?
Right now our empty states are vastly different from our loading and with-data states. We'd like to make the transition from loading to empty state smoother, as well as make the with-data state more familiar once a feature has been used. For list views, this means having our empty states within the context of the resource list.
WHAT is this pull request doing?
Currently,
ResourceList
only has an empty state for when there are no results for a filter or search query. This PR adds support for providing markup to render when there are not yet any resources to list. That way the same static content can be rendered in a loading, empty, and with-data state. See the tophatting instructions to view the smooth visual transition between these states using the playground code.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
git checkout refactor-and-revert-revert-resourcelist-emptystate
playground/Playground.tsx
yarn dev
to run and open Storybook locallyEmptySearchResult
should render (try searching for zebra, or filtering for the JPEG file type.)Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes