Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal/RFC for custom tag names #538

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented May 2, 2021

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:

Chrom Dev tools highlighting woo-gla-content-button-layout element

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.

To reduce the div-soup, and make use of modern browser features.
Example for: #84
@tomalec tomalec requested a review from a team May 2, 2021 14:16
@tomalec tomalec self-assigned this May 2, 2021
@tomalec tomalec changed the title Proposal/RFC for :custom tag names Proposal/RFC for custom tag names May 2, 2021
Copy link
Member

@ecgan ecgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 3 to 10
*/
import './index.scss';

const ContentButtonLayout = ( props ) => {
const { className, ...rest } = props;

return (
<div
className={ `gla-content-button-layout ${ className }` }
{ ...rest }
/>
);
};
const ContentButtonLayout = ( props ) => (
<woo-gla-content-button-layout { ...props } />
);

export default ContentButtonLayout;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! ❤️

);
};
const ContentButtonLayout = ( props ) => (
<woo-gla-content-button-layout { ...props } />
Copy link
Member

@ecgan ecgan May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

tomalec commented May 5, 2021

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
Copy link
Member Author

tomalec commented May 5, 2021

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
material-ripple styles 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
Copy link
Member

ecgan commented May 6, 2021

@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.

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to having this. 😏

Comment on lines +6 to +7
const ContentButtonLayout = ( props ) => (
<woo-gla-content-button-layout { ...props } />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) } />

Copy link
Member Author

@tomalec tomalec May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 } />
);

Screenshoot of the above in dev tools

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

@jconroy jconroy added the status: on hold The issue is currently not prioritized. label May 26, 2021
@eason9487 eason9487 changed the base branch from trunk to develop August 25, 2021 03:27
@tomalec
Copy link
Member Author

tomalec commented Nov 11, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on hold The issue is currently not prioritized.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants