Components: Apply width-based modifier classes to Placeholder only when width is known

[aduth]

Previously: #18745 (comment)

This pull request seeks to address change the behavior of the Placeholder component to avoid applying any of its width-specific modifier classes until after the first render when the width is known. This, combined with some reorientation of the placeholder layout in both #18745 and #19673 may have contributed to some intermittent end-to-end test failures, where the layout of the placeholder may change abruptly while the Embed tests are trying to click on one or the other of the "Convert to link" or "Try again" buttons.

 ● Embedding content › should allow the user to try embedding a failed URL again
    TimeoutError: waiting for selector "figure.wp-block-embed-twitter" failed: timeout 30000ms exceeded

With these changes, the width-based modifier classes of a Placeholder component (is-small, is-medium, is-large) will only be assigned after the first render, when the width is known.

Testing Instructions:

Repeat testing instructions from #18745.

Verify unit tests and end-to-end tests pass:

npm run test-unit packages/components/src/placeholder/test/index.js
npm run test-e2e packages/e2e-tests/specs/editor/various/embedding.test.js

[jasmussen]

Thank you for this. Tested out and things seem to work as intended:

Note, the placeholder for embeds isn't totally optimized for the new classes, but that doesn't mean the classes aren't there, just that more work can be done to polish the content of placeholders.

The first test passed for me too:

The second one did not:

[aduth]

The second one did not:

@jasmussen Interesting! I'm not able to reproduce that in my environment. But based on the error message alone, I could see how that might be a problem to try to select based on the ID, where the ID includes a number which isn't exactly deterministic. I think it would make sense to change that to one which is more reliable.

Out of curiosity, does the same test fail for you when you run it on the master branch?

[jasmussen]

Curious indeed. I moved to my home office and for whatever reason I can't run any of the tests. I'm getting a navigation timeout error, which does look remarkably similar to the one in the second image. Which to me actually suggests that the curiosity here is that some of the tests passed while in my coworking space. To phrase it differently, I may have a messy development environment, and you may want to get some more people to test this stuff.

[aduth]

I think it could be a combination of (a) yes, you might have something distinct to your environment and it might not actually be an issue with this pull request but also (b) the tests as written are fragile and can be improved.

[aduth]

the tests as written are fragile and can be improved.

See also: #19844

[aduth]

@jasmussen Would you be open to approving this? Otherwise, I see from #18745 that @jameslnewell had a hand in the original work as well, so I'll at least solicit the review in case there's any concern over the changes here.

[jasmussen]

The change looks good to me and if you're confident in the code, I see no problem in shipping this.

The only gotcha is that I wasn't able to get the tests to run due to my system, and I haven't had a chance to address that. So if you need someone else to sanity check, you can do that.

[FezVrasta]

Alternatively you could manually measure the element sizes and apply them as default values.

const [ resizeListener, { width = elementRef.current.clientWidth } ] = useResizeAware();

[aduth]

Hi @FezVrasta. If I understand correctly, I don't think that would be possible.

• A ref to a DOM node would only be assigned after the first render. It was my understanding this was also the reason react-resize-aware would provide null initially.
• The specific syntax wouldn't work, since the defaulting would not take effect if the value of width is null (only if it were undefined).

const sizes = { width: null };
const { width = 'somedefault' } = sizes;
console.log( width );
// null

[FezVrasta]

My bad, you are right, you would need an outer component with its own wrapping markup to do that.