Proposal/RFC for custom tag names

[tomalec]

This is an example for #84

Changes proposed in this Pull Request:

Add ContentButtonLayout a custom tag name.

To reduce the div-soup, and make use of modern browser features.

I believe such change could be valuable for the components that render a separate wrapping container. Like <div class="foo">, as opposed to components that render a document fragment, or to-be-collapsed element. As we render a separate HTML element, why don't we put some sematic into its name, instead of creating 100s of divs.

Please note, that this PR does not define a custom element - it should work in IE8+.
I use a custom-name casing, to suppress React warning for customname.

Screenshots:

Detailed test instructions:

1. Go to any page that uses ContentButtonLayout component, like https://gla1.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsetup-ads
2. Open dev tools, highlight the element.

Pros:

1. Better DevX:

   1. Less blurred picture with 1000s hypnotizing divs.
   2. Shorter rendered code <woo-gla-content-button-layout> vs <div class=".woo-gla-content-button-layout">
   3. Shorter working code update/84-custom-name?expand=1#diff-794cef5df7
   4. More semantic/informative HTML tree
   5. Makes it easier to find all instances of the rendered component with: document.querySelectorAll('my-component-name')

2. Works even without React. (if it's just a layout component as this one). No dependencies.

3. With the custom tag name, it is easier to relate it with React component, as opposed to reading the class names. See example below.

   // What we have currently:
   // it is not immediately clear this is rendered by which React component.
   <div class="gla-content-button-layout gla-title-button-layout many-more-class-names">
   
   // What this PR proposes:
   // from the custom tag name, you can guess it is coming from ContentButtonLayout component.
   <woo-gla-content-button-layout class="class-names-here">

Cons:

It's hard for me to find significant ones, please shoot if you have any.

1. It does not follow the existing "make everything a div" convention.
   • This is an argument we could put against any change, and I believe the current conventions do not bring much value themselves.
2. Component names me to collide and result in conflict
   • The same goes for the existing class name. So, this PR does not introduce any new risk.
   • The solution for the class names could more as well be adapted for the element name.
   • If we are about to modernize our HTML even further - go for custom elements, that issue will be solved by Scoped Custom Element Registries.
   • I believe the chances that somebody outside of this project would create a component prefixed with woo-gla- and called content-button-layout are pretty low.
3. If we would publish this component, and somebody would import it, <woo-gla-content-… name is not applicable.
   • Same goes for .gla-content-… class name.

[ecgan]

I like the changes that I'm seeing here. Just one uncertainty I guess: Do we want to merge this in and release this? Are we comfortable with this in production? If there is some sort of prior art within WooCommerce / WordPress, then that would give us more confidence on this.

[ecgan]

I got to say I like this, from 17 lines to 10 lines (and potentially even more for files that have imported and used classnames)! Really simplified a lot! ❤️

[ecgan]

I'm hoping / expecting this would work well with (possible future) CSS-in-JS solution, e.g. with emotion library (prior research: #71 ):

<woo-gla-content-button-layout
    className={css`
      padding: 32px;
      background-color: hotpink;
      font-size: 24px;
      border-radius: 4px;
      &:hover {
        color: ${color};
      }
    `}
  >

That would be great 🙂

[tomalec]

I haven't tried it with emotion. but I guess if emotion works for other elements div, p, span.

<div
    className={css`
      padding: 32px;
      background-color: hotpink;
      font-size: 24px;
      border-radius: 4px;
      &:hover {
        color: ${color};
      }
    `}
  >

I see no reason why it should work for all HTML elements.

For sure it works with native CSS-in-JS solution, like #539

[tomalec]

To elaborate on "... as opposed to components that render a document fragment, or to-be-collapsed element."

This proposal takes

ContentButtonLayout = () => (<div className="gla-component-name">...</div>);

And transforms it to

ContentButtonLayout = () => (<woo-gla-component-name>...</woo-gla-component-name>);

We can safely do that as the original <ContentButtonLayout> physically rendered an HTMLElement (<div>).

It will not work so straight-forward for a component like:

SpaceShipControl = () => (<ShipControl>...</ShipControl>);

As it does not render any HTMLElement. It renders virtual ShipControl, but there is no physical DOM HTMLElement being rendered for SpaceShipControl

Same goes for

FancyTiles = () => (<><div>...</div>...<div>...</div></>);

If we would forcefully wrap those in HTML elements, we may break their functionality.

---

Basically, we can only take the advantage of HTML & CSS features when we use HTML & CSS ;)
as opposed to React's virtual DOM, which lacks those features.

[tomalec]

Do we want to merge this in and release this? Are we comfortable with this in production? If there is some sort of prior art within WooCommerce / WordPress, then that would give us more confidence on this.

I'm not pushing for pushing this improvement before our tight deadline. I'm not aware of any prior art in WooCommerce / WordPress. But <customnames> are supported in browsers (HTML,CSS) for 8++ years now, and officially speced for 5+ as well.
Here you can see it being used in Google Ads panel

So from a browser-support point of view, I'd feel pretty convenient. The only concern I have here is WooCommerce code convention consistency.

[ecgan]

@tomalec , I have just thought of another advantage of using custom tag name: it is easier to relate it with React component, as opposed to reading the class names. Useful when looking at the browser dev tool's elements panel. I have updated the Pros section in your PR description, with a small code explanation example.

[eason9487]

Looking forward to having this. 😏

[eason9487]

One common confusion is that Web Components use “class” instead of “className”.
－ Quoted from https://reactjs.org/docs/web-components.html#using-web-components-in-react

Not sure if there is an easier way supported by react API to transform className to class, or we would need a helper like:

const cx = ( props ) => {
	if ( props.hasOwnProperty( 'className' ) ) {
		const { className, ...restProps } = props;
		return { class: className, ...restProps };
	}
	return props;
};

<woo-gla-content-button-layout { ...cx( props ) } />

[tomalec]

Thanks for catching this. Definitely, that's something we should be cautious about.

The thing is, we are not using Web Components here. woo-gla-content-button-layout is not a custom element. (it's a valid custom element name, but it's not customElements.define)

Therefore, I find it really strange that React does translate className to class for div, span etc, but not for any-other-element. Probably, that's and effect of simple check for custom-elements, and React's strategy for real custom elements facebook/react#4933, facebook/react#7901

That means if we would have to attach React event listeners to <woo-gla-content-button-layout> we would have to translate them as well.

Luckily, the children's elements are unaffected. So if we are about to cater only for a dummy <div className='gla-content-button-layout'> CSS layout wrapper, we're safe.

---

We can also try

const ContentButtonLayout = ( props ) => (
	<wooglacontentbuttonlayout { ...props } />
);

Then it's obvious that it's not a custom element. Then we do not need to open the "React vs. Web Standards" can of worms.
The only formal disadvantage of that, it does not pass HTML validation, even though the behavior is pretty reliable: https://stackoverflow.com/questions/10830682/is-it-ok-to-use-unknown-html-tags/27869027#27869027

[tomalec]

Given we no longer support IE, maybe we could revisit this idea.
I also made a PR that merges this with Shadow DOM styles scoping #1764