Fix screen flash when deleting templates in templates list

[annziel]

What?

Fixes #37926.

Without this PR, there is a screen flash when deleting templates from the templates list – an unnecessary disruption of UX.

How?

I made the templates list always visible when templates are available, even if (temporarily) not up-to-date.

Technically, I removed the isLoading condition for not displaying the templates list. Hiding the table is only useful on the initial load when templates is already null.

I don't think it makes sense to display a loader while re-fetching the data. It happens fast enough that hiding the table to display a loader would also look like a screen flash.

Testing Instructions

1. Create a new template from the Site Editor.
2. Head to the template list and delete it.
3. See no flash on the screen when the request is in progress.

cc @annezazu @youknowriad @adamziel

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @annziel! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[adamziel]

Looks good to me 👍

CC @youknowriad – I know you were thinking about a progress indicator in #37926 – what do you think about this approach?

[youknowriad]

yeah, this PR is exactly what I had in mind but I also thought it might be insufficient in the sense that in slow networks, you might click the button but don't receive any feedback immediately, thus the need for some kind of progress indicator. I don't feel so strongly about it though, let's see what designers say.

[jasmussen]

Thanks for the PR! A quick thought: can we show a spinner only if 0.4 milliseconds pass without the operation completing? I.e. an "optimistic" spinner that only shows once things are starting to drag on?

[annziel]

Thank you, @youknowriad!

Ok, @jasmussen, as you mentioned the "optimistic" approach, I thought maybe a better approach would be to remove the deleted template from the templates list without waiting for the fetch response? That wouldn't cause the temporary screen change or change the scroll position.

If not, then do you have any suggestions on how can I tackle the spinner display? What should I remove from the screen? Should I change the opacity of remained elements? And where should this spinner be placed on the screen? Is changing the scroll position a problem? Are there any guidelines available? That's a lot of questions, I know, but I don't know where to look for this information. Thank you!

[jasmussen]

I thought maybe a better approach would be to remove the deleted template from the templates list without waiting for the fetch response?

That sounds good, especially if you can animate/collapse during removal.

[annziel]

I've updated the PR (hope I did it correctly) by adding the code that removes deleted templates from the template list without waiting for the fetch response. Please let me know what you think about it. Unfortunately, I haven't found any good way to animate/collapse during removal. Is this addressed in other tables?
cc @jasmussen @youknowriad

[adamziel]

The code change looks great to me! @jasmussen, what do you think?

@youknowriad adding { excludeDeleted: true } as an option to useEntityRecords and getEntityRecords could be useful across the board – every React data table in WordPress could easily do the same as the one here. What do you think? It would even be a sensible default. Too bad both useEntityRecords and getEntityRecords are stable APIs.

[jasmussen]

Thanks for your patience with this PR, and appreciate the subtle optimizations here!

One issue, and it's entirely possible this is an issue with my local test environment. When I go into the site editor, into Templates > Manage all templates, I'm seeing an empty template list:

[youknowriad]

adding { excludeDeleted: true } as an option to useEntityRecords and getEntityRecords could be useful across the board

Personally, this feels a bit random to me. The solution in this PR is also not perfect since if the request fails, the template that was being deleted is restored at the end.

Also, the solution in the PR as it is now doesn't scale to other edits, we only perform "optimistic updates" for removal, what about "renaming templates"... In other words, I don't think it's worth adding complexity to show optimistic updates. That's why I suggested a loading indicator somewhere.

Your call though, I'm not a great fan of optimistic updates but I can accept it for deletions (not more) especially since it's collocated in the component.

[adamziel]

Personally, this feels a bit random to me. The solution in this PR is also not perfect since if the request fails, the template that was being deleted is restored at the end.

That's the downside of optimistic updates. I don't expect that to happen frequently, though.

Also, the solution in the PR as it is now doesn't scale to other edits, we only perform "optimistic updates" for removal, what about "renaming templates"... In other words, I don't think it's worth adding complexity to show optimistic updates. That's why I suggested a loading indicator somewhere.

You have a point – I was thinking that getEditedEntityRecord can already provide an optimistic mode for renaming templates, but now that I think about it more, getEditedEntityRecord reflects all edits and not just the ones that are being saved.

Your call though, I'm not a great fan of optimistic updates but I can accept it for deletions (not more) especially since it's collocated in the component.

I'm fine with either approach. It's just with deleting I can't think of a good location for a loading indicator and I'd rather not make the entire table disappear or fade out.

[annziel]

One issue, and it's entirely possible this is an issue with my local test environment. When I go into the site editor, into Templates > Manage all templates, I'm seeing an empty template list.

Oops, this should work now, would you try that again, please?

[adamziel]

@annziel This one looks ready to me; it just needs linting (the Static Analysis check fails).

[annziel]

@annziel This one looks ready to me; it just needs linting (the Static Analysis check fails).

Here it is!

[github-actions]

Congratulations on your first merged pull request, @annziel! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!