-
Notifications
You must be signed in to change notification settings - Fork 2k
MSD: Update the empty state for search results on both sites and domains #108439
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
64e00f5 to
cd634fe
Compare
|
Yeah, I think this is the right approach. I reached a similar conclusion after putting up #108433. There's more redundancy than needed, since we're swapping actions and conditionally rendering the Worth mentioning, I would prefer not call anything |
This is just a temporary name like CustomSelectControlV2 😅 Will rename it when we're able to remove the V1 |
|
This one seems better as we have different requirements of domain empty state for CIAB. See #108424. |
cd634fe to
24c9075
Compare
24c9075 to
545ae8b
Compare
| isLoading={ isLoadingSites || ( isPlaceholderData && hasNoData ) } | ||
| isPlaceholderData={ isPlaceholderData } | ||
| empty={ emptyState } | ||
| empty={ |
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.
Do we need to do the same thing to sites-ciab? Wondering because we are modifying domains-ciab in this PR 🤔
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.
Hmm maybe we can do it later; I don't know if the current actions are applicable to CIAB...
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.
Good question. We don't know what it should look like yet, I agree we can do it later.
fushar
left a comment
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'm seeing a flashing content when I searched in the Domains page for the first time:
B9fn7O.mp4
To reproduce, you need to Disable cache in your console, then hard reload the Domains page. Then start searching with bogus term to get to the empty search state.
I suspect it has something to do with this suspense boundary since it covers the PageLayout. 🤔
| const trackEmptyStateActionClick = ( action: string ) => { | ||
| recordTracksEvent( 'calypso_domains_dashboard_empty_state_action_click', { | ||
| action, | ||
| dashboard: 'ciab', |
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.
| <EmptyState.ActionItem | ||
| title={ __( 'Use a domain name you already own' ) } | ||
| description={ __( | ||
| 'Bring your domain to WordPress.com and manage everything in one place.' |
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.
Maybe a question for CIAB team (for later), do we really want to mention wordpress.com here 😄
Nice catch, yes. We can't wrap the PageLayout with |
55d64ef to
70713e1
Compare
|
I've moved the |
25c1b84 to
9387458
Compare
| <Suspense fallback={ null }> | ||
| <EmptyDomainsStateActions /> | ||
| </Suspense> | ||
| <EmptyDomainsStateUpsell /> |
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 haven't tested it but I believe we should put this one in the above suspense as well together, to avoid layout shift during the JS load 🤔
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.
To be honest, I don't like the lazy-load approach here. It kind of defeats the purpose of ensuring the data exists before rendering the view. I'll have to take a look at the context to understand why we need to lazy load this in the first place.

Part of DOTMSD-1067, DOTMSD-1068
Proposed Changes
DataViewsEmptyStateLayoutcomponent so we don't need to build the empty state from scratch for each viewDataViewsEmptyStateLayoutto DataViews on both sites and domains screenWhy are these changes being made?
Testing Instructions
Sites
Domains
Pre-merge Checklist