neighbour-hoods / design-system-components

Storybook and UI component library for the Neighbourhoods design system.
0 stars 0 forks source link

Decide on appropriate use of inheritance wrt `ancestor` component classes #4

Open pospi opened 1 year ago

pospi commented 1 year ago

I'm unsure whether strangeness in this inheritance chain is part of the reason there are bugs in component rendering; but it does seem like something we should revisit regardless. Perhaps a mixin-like architecture similar to what ScopedRegistryHost() does could be more idiomatic and composable?

NHComponentShoelace and NHComponentMaterial are currently defined differently and the latter appears not to have its own adapter styles present in any case.

nick-stebbings commented 1 year ago

Yeah, the NHComonentMaterial was sort of a stub in case Wes had any overrides we could use (as I think he is the only one using it currently) but can probably be removed.

Not sure about which bugs you mean with the component rendering. It is working ok in the Dashboard at the mo. What sort of issues have you been encountering?

adaburrows commented 1 year ago

I was thinking about this. It's not good practice to have a component to "be a" non-related thing. But it's more idiomatic for a component to "have a" set of associated styles. For example, in the dashboard, the table now reads to me as being a generic component derived from a base component as opposed to being a table.

Now, if it was named something other than it is which implies it is a table instead of having a table (like NhDashboardResourceView), then maybe I would not have had red flags go off.

I would strongly suggest not having a mix-in or inheritance pattern for styles in the general case for everyone. Rather, there should be plenty of ways for different applications to get at the styles to include in their application. A method I would strongly suggest is just having the variables loaded into the :root css context of the nh-launcher so all the variables are inherited in all the shadow DOM :host contexts. Then we could also have:

Then each kind of application can use whichever kind of idiomatic inclusion of CSS they would like. JSX using apps could just import the CSS modules and define the className attribute. Lit apps could just import the correct modules as JS template literals to be passed to ShadyCSS in an array.

If we want to use a mix-in in the dashboard, I'm not against that. But it should be something like:

function DecoratorForLitStyles(...styleList) {
  return function (Class) {
    if (Class.styles && Class.styles instanceof Array) {
      Class.styles = [...styleList, ...Class.styles]
    } else if (Class.styles && Class.styles instanceof Function) {
      Class.styles = () => [...styleList, ...Class.styles()]
    } else if (Class.styles) { // yeah, maybe check if it's a CSSResult, too
      Class.styles = [...styleList, Class.styles]
    } else {
      Class.styles = [...styleList]
    }
  }
}

const WithNhBaseStyles = DecoratorForLitStyles(baseStyles1, baseStyles2)
const WithNhShoelaceAdaptor = DecoratorForLitStyles(shoelaceAdaptorStyles)
// whatever else we need...

Then each component could just do

@WithNhBaseStyles
class NhAssessmentControl extends ScopedRegistryHost(LitElement) {
  static styles = css`
  :host { background-color: #000; }
  :host-context(h1) { border-bottom: 2px dotted white; }
  `
}

Then if components need to inherit from each other the prototype/inheritance chain won't be disrupted. It would also be made available to other people if they want to use our CSS with Lit.

pospi commented 1 year ago

@nick-stebbings

Not sure about which bugs you mean with the component rendering.

Don't worry, that turned out to be a red herring on CustomElement registration stuff.