Fix spinner causing a flash when loading site editor

[jasmussen]

What?

Working on a separate issue, I noticed that when you are loading the Site Editor and it features the Site Logo, the spinner component will briefly fill virtually the entire screen with a black circle.

Why?

The black circle happens because the CSS that styles the SVG spinner component, and indeed everything else, is not yet loaded. Observe:

How?

This happens because the SVG in the spinner did not have any explicit width/height applied, letting it scale with its aspect ratio to fill the width of the screen. This PR adds the same width and height back to the spinner, so at least it doesn't break boundaries:

Testing Instructions

Test loading the site editor, and be sure the content you load features a Site Logo block.

[jameskoster]

Are you certain it's the Site Logo block causing this? I'm not seeing the issue on trunk in a template that includes various blocks that utilise the Spinner (Site Logo, Navigation, Query).

The code looks fine, but if possible I'd like to reproduce the issue first to confirm.

[jasmussen]

Well, technically it's the Spinner component. But I reproduced it as seen in the GIF by inserting the Site Logo, which briefly loads the spinner in its initial state. I'm pretty certain of this. Quite possibly it's because I have a very heavy cache-clearing property set in my inspector, that dumps any cached CSS on every reload.

[javierarce]

I also couldn't reproduce the original problem, but the fix did improve the loading state by constraining the image to the proportions of the logo:

Before:

before.mov

After:

after.mov

[jasmussen]

Should we land this one? (✅)

[jameskoster]

I wonder if we should make the spinner size a prop, with 16 as the default, rather than making it static like this? That will be useful in contexts where a larger spinner might be desirable (e.g. #42525).

[jasmussen]

The problem is the default behavior of SVGs. If they have no width/height applied, they will fill all the available space. Our icons have default sizes as well for that same reason:

Think of it as a partner to the viewbox, but it's still meant to be overridden by the CSS size that, indeed, a prop could set.

[jameskoster]

Oh I agree that we should have a default size. I was just wondering if it should be possible to select from a range of sizes as a function of the component, rather than having to rely on custom css overrides. For the sake of consistency we might specify static sizes: "Small", "Medium", "Large", etc.

[jasmussen]

Right, I think that's still feasible, but as far as I'm aware, nothing currently breaks by adding this fallback value, and so it seems good to get the bugfix in, as it wouldn't prevent us from creating such props in the future.

Or is there a place where we're using the spinner large? I can't recall 🤔

[jameskoster]

It's being considered in #42525.

I'm happy to approve this, as it does fix the bug. But it would be good to get ahead of a potential proliferation of Spinner sizes :D

[jasmussen]

Thanks for the green check, as the flashing bug is really jarring on my end, I'm going to land it, especially as it doesn't preclude additional size options in the future. The spinner can even be sized up using CSS only if we want to go the hacky route in the mean time.