search

fabric-ds / elements

https://elements.fabric-ds.io
1 stars 3 forks source link

Element Design #2

Open digitalsadhu opened 3 years ago

digitalsadhu commented 3 years ago

Working with custom elements brings challenges that don't exist in the React world with regards to designing our components. This issue uses our breadcrumbs component to illustrate this.

The challenge of shadowdom, slots and Fabric/Tailwind

Consider the following component structure that uses Shadow DOM and slots:

<nav class="flex space-x-8"><slot></slot></nav>

Which is to be used like so:

<fabric-breadcrumbs>
    <a href="#/url/1">Eiendom</a>
    <a href="#/url/2">Bolig til salgs</a>
    <a href="#/url/3" aria-current="page">Oslo</a>
</fabric-breadcrumbs>

When all is said and done, the nav and slot element are in the shadowdom and therefore subject to any internal styles we use while the a tags are not in shadowdom and will be styled with the rest of the page as usual. The actual fabric-breadcrumbs element too, unlike react, IS an actual element on the page. IE. the component itself is not replaced with the root element (nav) as in the case with React. This means that to get the actual styled effect we want, the styles can't be on the nav element and instead must be on the fabric-breadcrumbs element like so:

<fabric-breadcrumbs class="flex space-x-8">
    <a href="#/url/1">Eiendom</a>
    <a href="#/url/2">Bolig til salgs</a>
    <a href="#/url/3" aria-current="page">Oslo</a>
</fabric-breadcrumbs>

Solutions

There are some design challenges implicit in trying to build such a web component to match the React/Vue implementations. There are a plethora of solutions to the challenge presented above, however, all feel like compromises.

Option 1 - Styling or class manipulation from within

One option is to try to style the component internally using special selectors ::slotted could be used to place styles on the a tags and :root could be used to place styles on the fabric-element itself.

It would also be possible to manipulate the classes on the fabric-element from within such that Tailwind could work.

Theres no shadow encapsulation available for the CSS in this approach and it would have to be either inlined style rules or reliance on an external Fabric style sheet. Far from what we are aiming for.

Option 2 - place the child elements within the shadowdom

It is also possible (P.O.C. example attached) to cut the child elements (the a tags) and paste them (using JS) into the shadowdom under the nav thereby subjecting the a tags to the same context as the nav and making internally loaded Tailwind/Fabric available. This makes the flicker effect of WCs worse but they need to be dealt with anyway.

Example implementation (without slots)

Content passed in is moved into a created Shadow DOM and / dividers are interleaved with the provided content.

export class FabricBreadcrumbs extends HTMLElement {
    connectedCallback() {
        const children = Array.from(this.children)
            .map((child) => child.outerHTML)
            .join('<span class="select-none" aria-hidden>/</span>');
        this.innerHTML = '';
        const shadowRoot = this.attachShadow({ mode: 'open' });
        shadowRoot.innerHTML = `
            <link
                rel="stylesheet"
                type="text/css"
                href="https://assets.finn.no/pkg/@finn-no/fabric-css/v0/fabric.min.css"
            />
            <nav class="flex space-x-8">${children}</nav>
        `;
    }
}

Option 3. - no shadowdom

Don't use shadowdom at all and let styling happen externally. Straight forward though we'd lose the encapsulation and the Fabric stylesheet would have to be loaded alongside the component for it to work.

Example implementation

No shadow DOM is used at all. Provided children are interleaved with / spans.

export class FabricBreadcrumbs extends HTMLElement {
    connectedCallback() {
        const children = Array.from(this.children)
            .map((child) => child.outerHTML)
            .join('<span class="select-none" aria-hidden>/</span>');

        this.innerHTML = `<nav class="flex space-x-8">${children}</nav>`;
    }
}

Option 4. - data passed in as attributes

We could pass data in as attributes. These must be passed a string due to how Web Components work which is very clunky but would I think solve all issues. eg. 

<f-breadcrumbs data='[["Eiendom","#/url/1"],["Bolig til salgs","#/url/2"],["Oslo","#/url/3"]]'></f-breadcrumbs>
digitalsadhu commented 3 years ago

After further in person discussion:

There are cases where we care about the encapsulation that Shadow DOM brings but it seems likely that our Fabric component library does not need to be one of them. Everything would be a lot easier with regards to our Tailwind based approach to Fabric if the components were open (no Shadow DOM) and just simply required that you had included the global Fabric stylesheet on the page.

This does not prevent encapsulation at a higher level. It would still be possible to create a podlet that is a web component with Shadow DOM that made use of these "open" components and in the process therefore encapsulated everything. This seems like good design to me.

digitalsadhu commented 3 years ago

FOUC Challenges

The FOUC (Flash of unstyled content) problem seems to be unavoidable in a satisfactory way. Take the following example element use:

<fabric-breadcrumbs>
    <a href="#/url/1">Eiendom</a>
    <a href="#/url/2">Bolig til salgs</a>
    <a href="#/url/3" aria-current="page">Oslo</a>
</fabric-breadcrumbs>

When the browser parsers the html, it will have no idea what a fabric-breadcrumb is and so will ignore it. However, it knows what a tags are and will treat them normally. However the page normally styles a tags will apply and so for the first few moments of page load, the user may see just a set of 3 a tags doing what a tags do. Shortly thereafter, the JS for the custom element will take over and do things with the a tags. In this case it will interleave / characters between them and so the content will render again creating a jarring flash from 

Eiendom Bolig til salgs Oslo

to

Eiendom / Bolig til salgs / Oslo

The only satisfactory solution is to try to hide everything until its ready but that's easier said than done. Nothing you do from within the component will work because of the same problem described above. ie. the component is completely ignored until its JS has loaded by which time its too late. This leaves us needing to hide the content in a way that's external to the component. Here we have essentially 2 choices.

  1. we can set a style inline on the component itself:
<fabric-breadcrumbs style="visibility:hidden;">
    <a href="#/url/1">Eiendom</a>
    <a href="#/url/2">Bolig til salgs</a>
    <a href="#/url/3" aria-current="page">Oslo</a>
</fabric-breadcrumbs>

This amounts to us needing to ask users of the component to set style="visibility:hidden;" in most cases. Yuck.

  1. we can hide all elements that are not yet ready 
:not(:defined) {
    visibility: hidden;
}

This is I think by far the preferred option. It's not perfect but it seems to do the trick and keep the initial unwanted render off the page. Once the JS has run and the custom element has been properly registered, the rule no longer applies and the component becomes visible.

digitalsadhu commented 3 years ago

The advantage of shadowdom

Another problem arises if we don't make use of Shadow DOM. That is, if we want to be able to apply styles to the component from within, we are unable to make use of the :host pseudo selector if we don't use Shadow DOM. The practical effect of this is that either:

  1. All custom elements will be displayed inline by default and users will need to override this manually
fabric-breadcrumbs {
  display: block;
}

or

  1. We need to set inline styles in the element from within the component
export class FabricBreadcrumbs extends HTMLElement {
    connectedCallback() {
        const children = Array.from(this.children)
            .map((child) => child.outerHTML)
            .join('<span class="select-none" aria-hidden>/</span>');
        this.innerHTML = `<nav class="flex space-x-8">${children}</nav>`;
        this.style.display = 'block';
    }
}

this is not ideal because setting the style in this way will also result in the style being set inline on the element directly like so:

<fabric-breadcrumbs style="display:block;">
    ...
</fabric-breadcrumbs>

Solving the problem with Shadow DOM

This problem goes away if we use shadow dom and the :host pseudo selector like so:

export class FabricBreadcrumbs extends HTMLElement {
    connectedCallback() {
        const children = Array.from(this.children)
            .map((child) => child.outerHTML)
            .join('<span class="select-none" aria-hidden>/</span>');
        const root = this.attachShadow({ mode: 'open' });
        root.innerHTML = `
            <style>:host { display:block; }</style>
            <link
                rel="stylesheet"
                type="text/css"
                href="https://assets.finn.no/pkg/@finn-no/fabric-css/v0/fabric.min.css"
            />
            <nav class="flex space-x-8">${children}</nav>
        `;
    }
}

We can use the :host pseudo selector to set the default display to block but its still possible for the user of the component to override this as needed.

benja commented 3 years ago

I think we should continue with the Shadow DOM as I believe the encapsulation of components are an important factor in making it as frictionless as possible to work with Fabric WCs. I've played around with a few solutions, and this seems to be where I stand right now (building on what we've discussed previously in person).

Example usage of component

<f-breadcrumbs class="mt-10">
    <a href="#/url/1">Eiendom</a>
    <a href="#/url/2">Bolig til salgs</a>
    <a href="#/url/3" aria-current="page"> Oslo </a>
</f-breadcrumbs>

The best way we've seemed to prevent FOUC is by hiding the visibility of a custom element until it has been defined / upgraded. We should put this logic, alongside the Shadow DOM implementation, behind a generic component that every Fabric WC extends.

Generic Fabric WC

export class FabricWebComponent extends HTMLElement {
    constructor() {
        super();

        const fabricStylesTemplate = document.createElement('template');
        fabricStylesTemplate.innerHTML = `
            <style>:host { display: block; }</style>
            <link
                rel="stylesheet"
                type="text/css"
                href="https://assets.finn.no/pkg/@finn-no/fabric-css/v0/fabric.min.css"
            />
        `;

        this.attachShadow({ mode: 'open' }).appendChild(
            fabricStylesTemplate.content,
        );
    }
}

Leaving us with a simple implementation of our Breadcrumb component:

import { FabricWebComponent } from '../../utils';

export class FabricBreadcrumbs extends FabricWebComponent {
    connectedCallback() {
        const children = Array.from(this.children)
            .map((child) => child.outerHTML)
            .join('<span class="select-none" aria-hidden>/</span>');

        this.shadowRoot.innerHTML += `<nav class="flex space-x-8">${children}</nav>`;
    }
}

We know FabricBreadcrumb is a Fabric component as it extends the FabricWebComponent. Hence, to further reduce redundancy, we could name the class Breadcrumb, and at element definition make sure to name the component properly following a f-COMPONENT-NAME structure.

This solution seems to be the cleanest to me right now. Thoughts on this?

digitalsadhu commented 3 years ago

This is really nice!

There is only 1 concern I can think of. If we create a base class, any component would need to choose between extending the base class and extending a different html element. Eg. If we wanted to extend HTMLButtonElement instead of HTMLElement for accessibility purposes.

For example:

export class FabricButton extends FabricWebComponent {}

may need to be changed to:

export class FabricButton extends HTMLButtonElement {}

and then do the wiring for Shadow DOM etc manually. Alternatively, we could create several base classes to be used for extending in the various cases. I'd guess that most cases do not need to extend anything other than HTMLElement. Button and maybe form inputs may be exceptions.

Another thought is that we maybe could extend HTMLDivElement instead of the generic HTMLElement in our base class to automatically get block level elements. Unsure if we want this but it would I think allow us to drop the :host {display: block} altogether perhaps.

benja commented 3 years ago

Great catch!

Did a bit of digging and found out that there are two types of custom elements: autonomous- and customised built-in elements. We are currently writing autonomous elements, which can only inherit from HTMLElement.

So we run in to the issue of not being able to inherit from a button, input, etc if we continue with autonomous elements, which to me seems to be the cleanest solution in terms of code-consistency:

Instead of <f-breadcrumb> we'd have to write <div is="f-breadcrumb">, as with a button <f-button> would end up being <button is="f-button">. I would also say the customised built-in element approach is more prone to error due to the nature of how they would be used. Although, the positives are that inheritance will work as expected.

If we continue with autonomous elements, the control lies in our hands. We can add button-like event handlers, etc. We would essentially have to implement the full-suite ourselves.

I'm currently looking for other possible solutions.

digitalsadhu commented 3 years ago

Right yea I somehow totally missed the requirement to use is=. That's a deal breaker. Autonomous components are defs what we want.