@dgwyer I'd love to get your thoughts on this first because I used your https://github.com/dgwyer/server-side-render-x repository as an inspiration for the change.

Size Change: +104 B (0%)

Total Size: 1.06 MB

_{compressed-size-action}

@ZebulanStanphill Thanks for the review and sorry that you were also working on this at the same time :(

I'm fine either way. I have addressed all your comments and tested it locally. :) So should be in a better state now 👍

@ZebulanStanphill Thank you for taking the time to review all of this. I have updated all the requested changes :)

Hi, @fabiankaegy

Do you have time to rebase the branch off of the current trunk?

@Mamaduka Looking at it now 👍

Thanks for the follow-up, @fabiankaegy.

It looks like I missed the notification, but I will try to review this tomorrow.

any recommendations regarding the styling of the loading state?

Thanks for the ping. Overall I think loading states across the board need a holistic rethink. The spinner needs love, and the states themselves could be looked at on whether they need spinners in the first place: if the loading happens fast enough, it's best not to show a spinner at all (i.e. the spinner could appear after a delay).

However the current loading state for the RSS feed is so mediocre that what's shipping already in this branch is an improvement, and since we need to look at loading states holistically, probably best to not hold up this PR for that.

I do share your instinct, though, that if there's prior art, it'd be nice to leverage that, and the prior art for the video is a good one to emulate. I.e. the entire item that is in a loading state becomes semi-transparent, and the spinner is overlaid dead center of the container. Same as for the image:

I updated the loading state styling to match existing ones in the core.

@fabiankaegy, @jasmussen, I would love to get your feedback if you think this is in a good state to land.

@Mamaduka I think that visual style works and is a great improvement over the current styling because we don't have that huge visual jump. I would be okay with shipping it like that. :) Thank you for your help! :)

From a quick glance at the video (thank you for providing!) I see the spinner centered in the box, which in its transient state is semi-transparent. Which seems good to me, thank you!

Good to see some progress on this. However, I'm not really a fan of the 'dimming' of the block content during the re-render cycle. I can appreciate why it was done this way. i.e. To make it easier to place the spinner in a consistent position central to the block.

Dimming though, introduces unecessary visual cruft for me. Especially for multiple quick fire changes in the block settings panel. I originally created my own version of the SSR component to remove as much visual change between the new rendered content and the previous content as possible.

e.g. This is my initial original effort. See how even for rapid settings changes the UI changes look almost as smooth as if it was being updated directly via JS.

Thanks for the feedback, @dgwyer.

I tried without dimming, but then Spinner isn't visible in some cases.

After this change, you should be able to override the LoadingResponsePlaceholder prop with a custom component and use styling that matches your block's UI. In addition, there will be no need to maintain the custom ServerSideDrender component since now you have access to the prevResponse as children prop.

Perhaps a separate exploration can be to only show the dimming and the spinner after a delay of a second or so, in an optimistic assumption that the operation will have finished before that.

@Mamaduka As I mentioned, it's obvious why the dimming was introduced but it doesn't really help solve the original issue. Ideally, no visual changes to the content transition should be introduced apart from the spinner. This was the primary motivation behind the original implementation.

I already addressed the issue of the spinner not being visible in some cases by providing a spinnerLocation prop to adjust the spinner relative location as required. Perhaps we can implement something similar here rather than introduce unecessary additional UI?

@jasmussen Debouncing might work. But I think being able to adjust the spinner location should be fine in almost all cases?

@dgwyer I would like to avoid controlling individual pieces of the component using props. It could easily increase the complexity of the component.

It's a little more code, but you could achieve the same results by overriding default loading placeholder:

function LoadingResponsePlaceholder( { children } ) {
	return (
		<div style={ { position: 'relative' } }>
			<div
				style={ {
					position: 'absolute',
					top: '10px',
					righ: '0',
				} }
			>
				<Spinner />
			</div>
			{ children }
		</div>
	);
}

<ServerSideRender
	block="core/archives"
	attributes={ attributes }
	LoadingResponsePlaceholder={ LoadingResponsePlaceholder }
/>

@Mamaduka Is there anything left that we need to do before this can get merged? Currently Merging is still blocked beause it needs approval.

@fabiankaegy See my comments above.

@dgwyer I saw your note but thought that it was addressed by the feedback from @Mamaduka here: #28289 (comment)

Because the ServerSideRender component now takes a LoadingPlaceholder component that gives you the ability to customize the experience to your liking this should give the flexibility to achieve the experience you are going after in core.

Debouncing might work. But I think being able to adjust the spinner location should be fine in almost all cases?

The postponed spinner feels like it could potentially benefit a lot of these blocks in a generic and reproducible way. The problem with spinner location is that you can't be sure any location is "safe", as it depends on the content inside and how it wraps. Since this PR is an improvement over what's shipping, it feels worth landing it and continuing conversation in new PRs and tickets.

The point is the defaut behaviour shouldn't have any additional layering over existing content during content re-rendering. The 'problem' of spinner location is easily mitigated by displaying it outside the block content. I've been doing this since the beginning and it just works (very well).

Adding UI on top of the content during render transition misses the point entirely. For JS only blocks the content is seamlessly updated, and that's what we want to achieve with SSR blocks too. My solution displays the spinner so it does not intefere with block content at all. See the GIF above (I've since update it so it displays to the left of the block container, vertically centered). This is a much better rendering experience for the user.

I've been using this daily for well over a year and have seen thousands of transitions. Dimming the content and adding the spinner right in the center of the block content during a render transition is not an ideal UX.

@jasmussen Outide of the block content can be considered a safe location.

I've been using this for a long time and it works very well. As you can see, rapid updating of attributes renders block updates very smoothly. Having the content blinking on/off continuously makes this a very poor UX and is why I implemented this solution in the first place.

Not necessarily. In the site editor the block might be flush against the top of the screen, which is why we moved the block toolbar below in #29464. Or you might be inserting the block in a narrow context, such as a column, where the block toolbar, or screen edge, will cover the spinner.

I do appreciate your point, and I do think it's worth following up with a postponed spinner. I think in many cases, certainly those you demo, there reaction time will be so fast there really is no need to see a spinner at all.

@jasmussen I'll reiterate my point for what it's worth and leave it at that. I would welcome the chance to make this even better for users.

The initial motivation for updating the SSR component was to match as closely the (re)rendering experience of pure JS blocks; which I achieved (with spinner location option). This merged solution does not do that in its current state.

As you pointed out we may not even need the spinner displayed at all in most cases. It would only need to be rendered as a fallback if there is a server delay.

This would be the best all-around solution as it would closely match JS block rendering behavior, fulfilling the original criteria.

Hi - I've been following this thread since I like what @dgwyer had done originally and I am in agreement that no dimming is preferable.

As a user experience it looks so much nicer to not fade out, it feels instant even though it takes the same amount of time. I'm disappointed this (appears to be) merged as is since it doesn't feel any better than what we have currently.

I created a new ticket, #35027, to improve the transient state so that it only appears if an operation takes a while. I'll see if I can ask a friend to work on it.