Skip to content

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Aug 22, 2025

Proposed Changes

The site list search box doesn't just search visible sites, it also searches sites which have been hidden. That is the site list empties when you start searching: it changes from a ?site_visibility=visible fetch, to a ?site_visibility=all fetch.

This PR changes the site list component so that retains the old data in the dataview when the new fetch is initiated. It is much less disruptive for most users who probably aren't looking for a hidden site.

Before

CleanShot.2025-08-22.at.17.02.01.mp4

After

CleanShot.2025-08-22.at.17.02.35.mp4

When you do have a hidden site

CleanShot.2025-08-22.at.19.53.08.mp4

Why are these changes being made?

Related to some product feedback pc4f5j-5KC-p2#comment-4989

While we can't improve the list performance until we've switched to elastic search, we can still improve the perceived performance.

Testing Instructions

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you tested accessibility for your changes? Ensure the feature remains usable with various user agents (e.g., browsers), interfaces (e.g., keyboard navigation), and assistive technologies (e.g., screen readers) (PCYsg-S3g-p2).
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@p-jackson p-jackson self-assigned this Aug 22, 2025
Copy link

github-actions bot commented Aug 22, 2025

@p-jackson p-jackson marked this pull request as ready for review August 22, 2025 05:23
@p-jackson p-jackson requested a review from a team as a code owner August 22, 2025 05:23
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 22, 2025
@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • help-center

To test WordPress.com changes, run install-plugin.sh $pluginSlug retain-sites-list-while-searching on your sandbox.

Copy link
Contributor

@arthur791004 arthur791004 left a comment

Choose a reason for hiding this comment

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

Nice 👍

@p-jackson
Copy link
Member Author

@arthur791004 I just uploaded a video of what happens when you actually are looking for a hidden site. It's not the best.

I still think we should merge it because it feels a lot better. But I wonder if we need to have another spinner for the site list @lucasmendes-design when we're fetching stuff, but it is happening in the background.

Like one of those indeterminate progress bars that snakes across the top of the card maybe.

Unsaved Image 1

Or maybe it could just be static text. I remember reading somewhere that originally Google Docs didn't have any saving indication because they felt one of their advantages was that "you never have to save on the cloud". But they got lots of user complaints because they didn't know where the save button was. So they ended up putting some static "loading" text in whenever a fetch was happening.

Unsaved Image 1

@arthur791004
Copy link
Contributor

arthur791004 commented Aug 22, 2025

I guess we can simply pass isFetching or isPlaceholderData to the isLoading prop of DataViews to resolve this issue

Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

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

What if we also preload the "?site_visibility=all fetch" here

loader: async () => {
// Preload the default sites list response without blocking.
queryClient.ensureQueryData( sitesQuery() );

@p-jackson
Copy link
Member Author

I guess we can simply pass isFetching or isPlaceholderData to the isLoading prop of DataViews to resolve this issue

Good idea. I've updated the PR and updated the video in the description to show what it does now when the search doesn't match any of the visible sites. This feels much better.

I still think the idea of a "fetching is happening in the background" indicator would be good too, because now if your search matches some of your visible sites, but you are actually looking for your hidden site, you have no indication that you need to just wait a couple of seconds before it will pop up.

What if we also preload the "?site_visibility=all fetch" here

It seems kinda heavy to do two requests IMO. A lot of the time I just land on the site list because it is the default view, I'm not looking to load gobs of data when I just wanna navigate somewhere else.

If we did want to have all that data client side then it would be better to just query site_visibility=all from the beginning and filter out the hidden sites client side.

@p-jackson p-jackson merged commit f99eacb into trunk Aug 25, 2025
12 checks passed
@p-jackson p-jackson deleted the retain-sites-list-while-searching branch August 25, 2025 02:31
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 25, 2025
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.

4 participants