Code quality: Remove unnecessary BaseControl usage from video block

[jorgefilipecosta]

Description

This PR applies a simple change to remove an unnecessary BaseControl usage in the video block.
It makes the video block code compliant with the lint rule being added on #14151.

I tried a different approach use the BaseControl as a label for the button being rendered inside it, but in my tests with a screen reader the button text stops being used and BaseControl label starts getting used, in this case, this change does not make sense, the button text should be used.

How has this been tested?

I checked that the Poster Image buttons still work.
I checked that no visual changes happen.

[aduth]

If we want a non-semantic block element, why not just render a div, rather than a span that we re-style?

[aduth]

Is BaseControl truly "unnecessary" when we had to recreate the margin it applies to the label it generates? This has a high risk of falling out of sync, and at the very least is only incidentally aligned to the base control margin (which is assigned by the value of the $grid-size-small variable, not hard-coded).

I realize in some ways this contradicts what's proposed in #9802 (comment) with having the "just another child" text element.

Maybe the compromise is: Leave it as BaseControl, render the label as a div / span, and effect the margin by some "padding between" all children of the base control field?

.components-base-control__field > * {
	margin: $grid-size-small 0;

	&:first-child {
		margin-top: 0;
	}

	&:last-child {
		margin-bottom: 0;
	}
}

[jorgefilipecosta]

Hi @aduth the code was updated following a logic similar to the one you described.
The specific CSS rule is a little bit simpler and just applies the margin to the first child of components-base-control__field, it seems safer e.g: Toggle control works exactly as before while if we apply "padding between" all children the design get broken.

[aduth]

This has me questioning a bit the proposal considered for the ESLint rule, since we have some styling to apply to a label, whether that label is the <label> element, or one of these visual labels. It doesn't feel very stable to rely on some convention that the first child would be the label element.

It makes me wonder if we should keep allowing label to be provided as a prop without id, but enforce it as a special case which needs to be consciously opted-in to:

• Using a different prop name which clearly identifies it as a visual label, e.g. visualLabel ?
• Have a separate boolean prop which is considered by the ESLint rule to allow it as an exception, e.g. hasVisualLabel, hasOrphanedLabel
• Keep using it as we do today, and in adding the ESLint rule, intentionally disable the rule where exceptions are okay, using eslint-disable

cc @afercia

[aduth]

Suggested change

[afercia]

Preferably, I'd like the Editor to enforce a proper usage of visible <label> elements. I don't think the component should take into account the styling of "visual labels". Instead, not providing a default styling for visual labels would encourage developers to use real labels 🙂

Alternatively, this option:

Have a separate boolean prop which is considered by the ESLint rule to allow it as an exception, e.g. hasVisualLabel, hasOrphanedLabel

should also make sure the input if labelled, wither with an aria-label or an aria-labelledby that references an existing ID. I realize it gets more complicated: one more reason to enforce only the first case?

[jorgefilipecosta]

What if we have a BaseControl.VisualLabel component that contains the correct padding and can be used for these cases as a child component?
It would be just a child component, and it is not as practical to use as a label prop so it would only be used for this type of exceptions.

[aduth]

@jorgefilipecosta That's a pretty clever idea!

[jorgefilipecosta]

Hi @aduth thank you for analyzing this hypothesis, I updated this PR to implement this idea so we can get a better feel of how it looks.

[aduth]

Two notes:

• If the original idea was that BaseControl could use this visual label in the default implementation, should that be done here? In other words, I don't see a reason why we need multiple styles selectors to cover both .components-base-control__label and .components-base-control-visual-label, when it should be the same (replace <span className="components-base-control__label"> above with <BaseControl.VisualLabel>).
• If there's a worry about backwards-compatibility, we should consider just applying the existing .components-base-control__label. In fact, I'd go so far as to suggest this is not compliant with the standard recommendations (it is not in a folder base-control-visual-label). While it is its own root component (which complicates things), I think it's fair to say this would qualify as a descendant of BaseControl, and thus .components-base-control__label would be a correct/ideal class name anyways.

(Aside: I have been meaning to propose some improved phrasing for the naming recommendations, as they're admittedly a bit unclear and editor-oriented at the moment)

[jorgefilipecosta]

Hi @aduth, thank you for the suggestion I am now using the class components-base-control__label.

If the original idea was that BaseControl could use this visual label in the default implementation, should that be done here? In other words, I don't see a reason why we need multiple styles selectors to cover both .components-base-control__label and .components-base-control-visual-label, when it should be the same (replace above with <BaseControl.VisualLabel>

I'm not sure If I understood correctly. Should we use BaseControl.VisualLabel inside BaseControl itself to replace <span className="components-base-control__label"> ? Or the current code is correct and we just need to use the same class?

[aduth]

Should we use BaseControl.VisualLabel inside BaseControl itself to replace ? Or the current code is correct and we just need to use the same class?

The first is what I'd imagined. Though, in retrospect, I suppose the idea is that we'd soon remove that span altogether, preferring instead that VisualLabel be used? I guess the argument could be made that it must be kept for compatibility (at least to not render a label, the worst case). I'd think if we keep the span, it probably makes sense to render VisualLabel in its place, since they effectively represent the same thing. Would you agree?

[jorgefilipecosta]

Hi @aduth, I updated the code to follow your feedback. Let me know if I understood everything correctly, or if missed something. Thank you for the feedback.

[aduth]

Minor: We don't have much guideline around JSX formatting, but in my experience I tend to see mostly one of either:

<div className="its-a-single-line">

Or:

<div
	className="its-multiple-lines"
>

Where if in the multi-line, the value of the attribute spanned multiple lines, it would look more like:

<div
	className={
		'its-even-more-lines'
	}
>

In this case, I think all of it could be avoided by just assigning the result of classnames into a variable, depending on our level of comfort with overriding the value of the className prop:

className = classnames( 'components-base-control-visual-label', className );

return (
	<div className={ className }>

I don't feel very strongly about any of this, though I've thought on a few occasions whether it's worth starting a discussion on more rigid conventions around JSX formatting.

[jorgefilipecosta]

Hi @aduth,
I agree maybe we can analyse a stricter JSX formatting rule.
Although we normally prefer this formats:

<div className="its-a-single-line">

<div
	className="its-multiple-lines"
>

The other format has an advantage regarding internacionalization -- I think it is the one that guarantees translator comments correctly reference the string.

<MyComponent
	mySpecialLabel={
		// the translator comment
		__( 'my special label' )
	}
>

I noticed it is even safer than using variables before e.g:

// the translator comment
const mySpecialLabel = __( 'my special label' )

Because during the transpiling the variable declarations may be reordered/set in a different way.

But in this case, the usage of a variable is the one that makes more sense. The code was updated.