woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

Investigate the use of custom tag names for React Components #84

Open tomalec opened 3 years ago

tomalec commented 3 years ago

It's quite a high-level idea, for DevX improvement. Probably, could be discussed with a broader audience.

Problems

Currently, all our React components are returning a <div> as a top-level element. To style it the component we use a class, like <div class="woocommerce-dashboard-section">.

That introduces several problems:

  1. We end up with a meaningless div-soup
    • makes the resulting tree harder to read, debug and maintain divsoup in a browser dev tools
  2. It's not clear whether the given CSS rule is meant to style all instances of the component being defined or a group/class of elements.
  3. To style the child/sub-components our component uses, we need to know the child component implementation details or sprout more class names everywhere. Consider following example. Woo's ProfileWizard uses external component TabPanel from @wordpress/components, and wants to set some styles for the TabPanel element. To achieve that it either

    • needs to know and hard-code the implementaiton detail of TabPanel: the .components-tab-panel__tabs used there
    • put a class name on <TabPanel>

    What's even more confusing, in this particular case it does both :confused: Why?

Proposal

I think we can improve it by using a component's name in HTML as well. So we will have a more 1:1 relationship between the JS and HTML code. The code we write and the code that is executed in a browser and we the A8C developers, HEs, and our users debug.

We can simply render an element with a custom name, like <woocommerce-profile-wizard>.

:warning: Please note, that we do not have to define any custom elements.

Its behavior is defined as HTMLUnknownElement and HTMLElement. and it works well even in IE9. You can check how it works in your browser here https://jsbin.com/wupinok/edit?html,output.

Benefits:

Potential problems and solutions:

Risks, to be checked and evaluated:

ecgan commented 3 years ago

@tomalec , hey this looks really cool! First thing that came to my mind is some time ago I read that react and custom elements do not work well together, I'm not sure how true it is today though. Also, I think that there is a way to put namespace for custom elements like <woocommerce:faq-list>, right? This might help to avoid collision.

Lastly, do you know if there is prior art or discussion on this within a8c? That would help to determine whether this is a go or no-go.


see working example https://jsbin.com/zohewew/edit?html,js,output

Btw, there is no output in this jsbin.

image

jconroy commented 3 years ago

Thanks for posting @tomalec this is a great overview of your thoughts!

For now, let's follow suit with the current approach (acknowledging there is room to improve) and revisit once we have less time constraints.

tomalec commented 3 years ago

@ecgan

I read that react and custom elements do not work well together

AFAIK, that's the thing with React and any new ES or HTML feature ;) Seems to be the price of any big framework that creates its own platform. I hope it's better now, given, the Web Components are around for ~8years, and for ~3yrs supported in major browsers.

I think that there is a way to put namespace for custom elements like , right? This might help to avoid collision.

<namespace:*> is XML/XHTML thing, and is a separate topic. I think it never got traction and is not very popular. The common and advertised approach to namespace custom elements was the convention to call them, like <ventor-element>, that's one of the reasons why the dash (-) is required for a custom element name. That also guarantees the lac of collision with new native elements that may come in the future, which will be always without dash. (yeah, yeah standard bodies regred, that there are some legacy native elements with dash, (but the Web have to be backward compatible.)

do you know if there is prior art or discussion on this within a8c?

I did some brief research and didn't find anything, If I get more dime, I could dig deeper. I asked @alshakero who is in A8C longer than me and he is very experienced with Web Components as well.

Btw, there is no output in this jsbin.

:( JSBin still have some problems with saving revisions. Is it better now? if not you may try https://jsbin.com/zohewew/4/edit?html,output

ecgan commented 3 years ago

Btw, there is no output in this jsbin.

:( JSBin still have some problems with saving revisions. Is it better now? if not you may try https://jsbin.com/zohewew/4/edit?html,output

Yup, both links are working now! 🙂 👍