reapit / elements

Reapit Elements UI Component Library
https://elements.reapit.cloud/?path=/story/welcome--page
0 stars 1 forks source link

Spike: V5 format proposal #124

Closed willmcvay closed 2 weeks ago

willmcvay commented 1 month ago

Background context or User story:

As discussed with Kurt and Karmen, proposing that we adjust the structure of how the project is presented in Storybook for v5. This ticket is a spike so we have something to discuss but if we like, can use as the model going forward

Specification or Acceptance Criteria:

Broadly, we identified 3 categories of story and the proposal is to group and structure by these flavours:

Currently the Storybook is a jumble of all 3 - suggest we make these distinctions clearer and this spike as a proposal we can discuss, refine and potentially adopt

kurtdoherty commented 3 weeks ago

@willmcvay Thanks for putting this together along with the demonstration in #125 👏 I haven't looked too closely at the file structure in #125 yet, as I'd like to focus first on the definition of "basic components" you've used above.

Basic Components [snip] - Presentational wrapped Linaria tags that generate vanilla HTML and CSS, suitable for use as CSS classes in any app (inc non-React). Composable, more un-opinionated - allows maximum flexibility if required.

When I think about designing a component that is made up of smaller parts—like accordions that may consist of an AccordionGroup, Accordion, AccordionHeader and AccordionContent—I want the flexibility to use context, or dynamically determine what HTML element is rendered, within each of those smaller parts so that I provide a simple, minimal and intuitive API for developers to use.

Perhaps I'm missing some aspect of Linaria's API, but it does seem like this would be possible if we were exporting Linaria styled components as these "smaller parts" for use by developers. Firstly, Linaria styled components seem to be explicitly tied to a specific HTML element (e.g. styled.button) and, secondly, don't seem to allow for the styled component to consume from a React context unless one does styled(AccordionHeader) (which I wouldn't be in favour of).

As such, I'm concerned about us equating Linaria styled components with the "basic" or "atomic" components that we would be exporting for developers to use directly.

I need to keep thinking about this, as well as look at the rest of your proposal, but wanted get some discussion underway. Keen to hear what others think (I'm happy to provide more details on the above, like code examples, if that'll help everyone understand what I'm trying to get at).

willmcvay commented 3 weeks ago

Hi @kurtdoherty so I think I understand your question (and apologies if not!), but looking at the PR the diff looks a bit jumbled so will try and clarify what I am proposing. Obviously, there are 2 scenarios, basic and composed components.

In most cases, most devs will probably use composed components as they will handle the happy path but of course, we want to know we can break out if there is a non-vanilla use case. Some code examples:

Basic:

Each of the linaria classes are wrapped and exported as atoms here: https://github.com/reapit/elements/blob/3f642935482b9c45edc929d98c21ac1567999f24/src/components/accordion/basic/accordion.tsx Then can be composed as you like here: https://github.com/reapit/elements/blob/3f642935482b9c45edc929d98c21ac1567999f24/src/components/accordion/basic/accordion.stories.tsx

In this scenario, if you want to override a style, you can just add a class to any of the atoms using the className prop, from whatever framework you like - css modules, sass, linaria, whatever - and they will be passed down to the underlying markup. Max flexibility.

Composed:

We do the heavy lifting for you by adding the standard JS behaviour and vanilla markup in the composed component here: https://github.com/reapit/elements/blob/3f642935482b9c45edc929d98c21ac1567999f24/src/components/accordion/composed/accordion.tsx The things that change are just passed as props and handled by the composed component - it "just works" for the happy path: https://github.com/reapit/elements/blob/3f642935482b9c45edc929d98c21ac1567999f24/src/components/accordion/composed/accordion.stories.tsx

In this scenario, far less code but also way less flexibility.

Overall:

My thoughts are we offer both options for all components where it makes sense to do so. Components that are intrinsically "basic" or "atomic" like say buttons wouldn't have a composed version because there is no value in over-complicating.

Hopefully this makes sense - will get some time this week to adjust the storybook structure as discussed as a separate concern.

kurtdoherty commented 3 weeks ago

@willmcvay thanks for the reply. I think there's some misunderstanding of what I'm poking at so I'll try to explain again, but we may need to chat verbally about this. Ultimately, I think a lot of this is touching on a larger conversation around component design patterns and how we balance flexibility and consistency in Elements.

I'll summarise my thoughts below:

  1. I don't think our definition of a "basic" component should require it to be a Linaria styled component, though this seems to be what you are proposing. It may work out that way sometimes, but my gut says that won't be very common. The Linaria styled components are not the same kind of component as what I was talking about in that conversation with Karmen; they are categorically different;
  2. Prefixing Linaria styled components with El, like ElAccordionTitle, is good. It helps separate them from all the other React components we'll have. I think this is good, because (again), I think Linaria styled components are a different category of component than all the other kinds of React components we'll be implementing;
  3. I don't think there's much value in distinguishing between basic and composed components (so long as we still distinguish between Linaria styled components and every other React component).
    • I think the difference between a "basic" component and a "composed" component is more about which component design pattern was applied when the component/s were built (and the reasons behind that decision);
    • Some components may be singular, "all-in-one" components like the current Accordion; others may be made up of smaller parts that are composed together by consumers. I'd recommend we table the various design patterns for discussion soon. That we can all start speaking the same language and have the same understanding of the flexibility/consistency tradeoffs inherent in the different patterns.

Hope my perspective is clearer now; if not, let's book in time to talk through it verbally. That way we can point at real code etc.

kurtdoherty commented 2 weeks ago

@willmcvay have looked over the rest of the changes and digested some more of the "class names are a public API" consequences. Just have two more non-blocking suggestions:

suggestion: Maybe Basic usage could be CSS usage or CSS-only usage? Seems like that's what we're showing.

suggestion: I don't love that we're exporting things from folders named __styles__. The double underscore is typically a signal that something is private, yet in this case, the exports from the folder define the public CSS API surface for a component. I wonder if we could just use css or styles instead?

If my first suggestion is implemented, I'd lean towards using css as the folder name so that we create a clear relationship between the storybook CSS usage examples and the code backing that usage for devs, but styles seems fine and would be more familiar to the existing team.

willmcvay commented 2 weeks ago

@kurtdoherty Seems reasonable - TBH, I'm struggling with meaningful naming conventions.

Think CSS Usage could be a bit confusing since we are still exporting React components, albeit with extracted styles to a CSS stylesheet. Am thinking Styles Only Usage - thoughts?

Yes, fine with moving away from __styles__ as a directory name - for the same reason though, css seems odd since would be a single .ts file for each component. Am thinking just a styles.ts file for each component - again, thoughts?

Will adjust the PR today and give you a nudge to take a look.

kurtdoherty commented 2 weeks ago

I don't have a strong opinion here. Both are reasonable options, so if your gut says styles, lets use that 👍

I only leant towards css as the folder name because I'd specifically suggested CSS usage on the storybook side. This was mostly on the premise that our export of the raw Linaria styled components doesn't really seem necessary. If you're using React, you'll be going for the "real" React components (e.g. AccordionHeader, not ElAccordionHeader). If you're not using React, you're interested in CSS class names, so CSS usage seemed like a clear way of communicating "this is what you care about". That said, I think style/styles works fine too 🤷‍♂️