daegren commented 3 years ago

This API is getting a little weird. This prop overrides all the other props, right? That makes me want to make it its own TitleText component instead, which also has the advantage of avoiding the Variant type and the getVariantStyles function altogether. If you intend to extend with more variants, they can also be their own components.

const TitleText = styled.span`
  ${getSizeStyles('2xl')};
  color: ${({ theme }) => theme.color('green500')};
  font-family: ${({ theme }) => theme.fonts.mono};
  font-weight: 400;
  text-transform: uppercase;
`

Originally posted by @david-crespo in https://github.com/oxidecomputer/console/pull/97#r599695620

david-crespo commented 3 years ago

Mixin/helper version of Text discussed here: https://github.com/oxidecomputer/console/pull/141#discussion_r601553595

daegren commented 3 years ago

Should've been discussed here first @david-crespo

paulathevalley commented 3 years ago

Clarification Request: I expected size="base" to be the default setting for Text but on further inspection, it is not the default. Is this a bug or intentional? 😬 I cannot recall the details.

See: https://github.com/oxidecomputer/console/blob/main/libs/ui/src/lib/text/Text.tsx#L23

I was expecting not to see that early return and see something like:

  case 'base':
  default: 
    return css` ...

or even

${({ size }) => getSizeStyles(size || 'base')}`

https://github.com/oxidecomputer/console/blob/main/libs/ui/src/lib/text/Text.tsx#L168

Could this be clarified here as well?

david-crespo commented 3 years ago

Did an audit of how we use Text right now. Excluded specs and stories.

style	uses	search pattern
`styled(Text)`	19 + 5-6 inheritances	`styled(Text)`
`<Text>`	8	`<Text`

We're using as: in 12 of the 19 styled(Text) uses.

attr	uses	search regex
`size`	28	`[^a-z\-]size[=:]` manually excluding Button ones
`color`	11	`[^a-z\-]color(="\\|: ['"])` manually excluding Icon ones
`variant`	2	`variant: 'title`
`weight`	1	`/[^a-z\-]weight[=:]`
`font`	0	`[^a-z\-]font[=:]`

daegren commented 3 years ago

I think an exploration of the wanted API would be a good first step. I think we more or less have two ways of approaching this API (without thinking of changing the base architecture and going a non-css in JS route), which really don't matter the way it's implemented as long as it's consistent.

One way we've been thinking about this so far is to style Text at the leaf nodes where the text actually lives in the hierarchy, I will elaborate more on this approach in the rest of this post.

The other approach is to apply styling to containing elements and rely on the cascading nature of CSS to propagate the styles. I haven't dug really into this approach too much, or how this API might look like, and if it merits digging into I can do so in a follow-up post. However, one immediate drawback to this approach that by styling nodes in the hierarchy instead of the leaves, we can run into the same issues we'd have with plain CSS classes and cascading. By styling the leaf nodes, we will be sure that our elements are properly styled, and won't get affected by weird compositional issues.

API Goals

When @paulathevalley and I first came up with the <Text> API it was with a few goals in mind:

Clearly indicate that text is styled in a design-system approved manner.
Have a clear way to create specific variants of Text
Have a clear way to compose Text with other components, e.g. Icons

APIs

Text as Component

This is kind of our current pattern, and hits on the main goals:

Using a <Text> component is clear that it's going to be styled properly to conform to the design system.

const Component: FC = () => (
 <>
   <Text>This is some text!</Text>
   <Text size="sm">This is some small text!</Text>
   <Text color="white">This is some white text!</Text>
   <Text color="white" size="lg">
     This is some large white text!
   </Text>
   <span>This text is not guaranteed to conform to the design system</span>
 </>
)

There are a couple ways to satisfy variants:

We can provide a variant prop on <Text> which changes the styling to match common variants

const Component: FC = () => (
<>
  <Text variant="title">This is a title</Text>
  <Text variant="menuItem">This is a menu item</Text>
</>
)

Alternatively we can export new components which provide default props:

import { TitleText, MenuItemText } from 'libs/ui/text'

// Usage
const Component: FC = () => (
<>
  <TitleText>This is a title</TitleText>
  <MenuItemText>This is a menu item</MenuItemText>
</>
)

Although this approach detracts from goal 1, I still wanted to include it here for discussion.

Composing it is just a matter of creating new components

const IconTextContainer = styled.span`
 display: inline-flex;
 flex-direction: row;
 align-content: center;
 justify-items: center;
`

const IconWithTitle: FC<Pick<IconProps, 'name'>> = ({ name, children }) => (
 <IconTextContainer>
   <Icon name={name} />
   <Text variant="title">{children}</Text>
 </IconTextContainer>
)

// Usage

const Component: FC = () => (
 <>
   <IconWithTitle name="profile">My Profile</IconWithTitle>
   <Text variant="menuItem">Menu Item</Text>
 </>
)

Other considerations

`children` as `ReactNode` or `string` vs `value` prop

Given the way we're treating our Text component as a styled TextNode at the end of the day, I'm curious what the implications of using children which is a ReactNode vs re-typing children as a string vs just passing a string as a value prop and render that inside the styled span.

const Component: FC = () => (
  <>
    <Text>
      <p>This is some text</p>
      <p>That is separated into paragraphs</p>
    </Text>
    <TextStringChildren>
      <p>This is not possible</p>
    </TextStringChildren>
    <TextStringChildren>
      This is a valid version of a single string child
    </TextStringChildren>
    <TextValue value="This is a value prop version" />
  </>
)

Given that we might need to group text into paragraphs or some other DOM elements, I think we should stick to using children as is for now.

Working with the wrapping `<span>` and `styled(Text)`

For the most part, it appears that we're using this pattern to add margins/paddings, apply text-transforms, or to just avoid passing props in the JSX using .attrs().

If we don't like this pattern and want to remove it there are a few options:

Use plain styled elements to wrap the text and apply the margin and/or padding styling to that element.
```
const TextContainer = styled.div`
 margin: ${({ theme }) => theme.spacing(8)};
`

const Component: FC = () => (
 <TextContainer>
   <Text size="sm" color="green">
     This is text with some margin
   </Text>
 </TextContainer>
)
```
This does add extra nodes to the DOM, and might be a good reason to avoid this pattern, but it keeps a clear separation from the "text" styling and the spacing around the text

text-transforms and other common CSS patterns can be made part of the <Text>'s props

const Component: FC = () => (
 <>
   <Text size="sm" textTransform="uppercase">
     small shouting
   </Text>
 </>
)

~Limit use of styled(Text) and styled(Text).attrs({})~

This is an implementation detail and detracts from the conversation about the desired API

Caveats

Implementation details are intentionally left out, this discussion should be about the API we want, not how it works internally (yet).

david-crespo commented 3 years ago

3 (the issue of attrs) is not an implementation detail. It is about use of the component. If the API as described requires you to use attrs 3/4 of the time (as indicated by my audit), it may not be satisfying the design goals because the API in theory (<Text ...>) is not the same as the API in practice:

const MainDataValue = styled(Text).attrs({
  color: 'gray50',
  size: 'xl',
  role: 'cell',
})``

david-crespo commented 3 years ago

The addition of CSS mixins to handle the 3/4 of cases where we are currently using attrs does not mean we have to get rid of Text for the spots where it's convenient and neat, which I agree it is.

Proposed CSS API

const StyledCard = styled.article`
  color: ${su.color('green50')};
`

const Title = styled.div`
  text-transform: uppercase;
  ${su.textSize('lg')}
`

const Subtitle = styled.div`
  ${su.textSize('sm')}
`

Above is extracted from full example of proposed API here.

Variant API example

const PageTitle = styled.h1`
  ${su.titleText}
`

Extracted from description in https://github.com/oxidecomputer/console/pull/163

paulathevalley commented 3 years ago

Those API goals still look great. 👍

Re: Text as Component: This is the approach that I’d like to (continue to) use.

Generally speaking, I am partial to the approach of analyzing how Text is being used in order to inform what additional variants or styles should get formalized in the design system. In other words, first analyze usage of Text before adding additional sizes, styles, or variants to Text. The example of text-transform at the very end of @daegren’s post describes this process perfectly: it appears often, so it’s time to add it to Text. I would probably modify it slightly by not exposing too many details of how CSS works, and instead supporting text-transform: uppercase; with an uppercase: boolean prop, because this is more specific to the intended use.

I am partial to adding props to Text to hold common patterns, e.g. <Text variant="title">, <Text uppercase>, etc. This means that visual changes do not require adding or removing import statements (as you would need to do for TitleText), but it is really not that big of a deal. I do not mind seeing these styles in the JSX, but I do understand how separating them out completely can help the reader determine what is visual styling vs what is structural/functional.

As for composition, my mental model expects Text to be flexible via its props, because it’s not always clear to me as a reader that the components: TitleWithIcon, TitleText, Text all return styles related to text and do nothing else. In my mind, they are technically separate components, which means they have the ability to do whatever a single react component can do (which is a lot). So, I'd have to get familiar with the internals of all the composed components in order to verify that they simply change styles. This mental model drives my expectation for Text to be a singularly robust component that can handle any variant or text-related composition itself. All the text styles are in a single place, with a single docs page outlining the boundaries of what it can do. My mental model will be satisfied as long as all text-related components are documented in the Text docs page in storybook. It allows a 'single source of truth' as it relates to Text and that's all I really need.

paulathevalley commented 3 years ago

My issue with using style utils only is that it does not satisfy the first API goal as clearly as a Text component does.

<Text variant="title">

is much clearer that it is design-system approved than

const PageTitle = styled.h1`
  ${su.titleText};
  // potentially other CSS that overrides `su.titleText`
`
// ...a lot of lines later...
return (
  <PageTitle>

I propose allowing <Text> to use style utils via its variant, uppercase, etc. prop API. Potentially other components in the design system could rely on some of these style utils as well, but I have a strong preference for the app to always being drawing components from the design system instead of creating its own styles.

david-crespo commented 3 years ago

I agree, I like keeping Text for cases where it helps you see everything inline.

daegren commented 3 years ago

3 (the issue of attrs) is not an implementation detail. It is about use of the component. If the API as described requires you to use attrs 3/4 of the time (as indicated by my audit), it may not be satisfying the design goals because the API in theory (<Text ...>) is not the same as the API in practice:
const MainDataValue = styled(Text).attrs({
  color: 'gray50',
  size: 'xl',
  role: 'cell',
})``

It is implementation details as all this is doing behind the scenes is rendering a component with those props, so .attrs() equivalent to:

<Text color="gray50" size="xl" role="cell">Text</Text>

and styled(Text) is just passing in a class via a className prop, which are both just extensions on top of the React component API.

david-crespo commented 3 years ago

The fact that it means the same thing behind the scenes is an implementation detail. The fact that you have to write styled(Text).attrs({ is the secondary (but as I argue, in practice the primary) API of the text component.

paulathevalley commented 3 years ago

I agree it is an implementation detail, because styled(Text).attrs({ color: 'gray50' }) is equivalent to <Text color="gray50">, so it's just a matter of choosing which way it’s written in the app, not how the component API is structured in the ui library, which is what we’re discussing here.

Does this mean we’re all good with continuing to use <Text /> as component? That’s a good first step!

I also see no objections to adding support for text-transform: uppercase; to <Text /> as that's been commonly used in the app. I proposed using an uppercase prop for the reasons I listed above, but I'm open to textTransform prop if anyone feels really strongly about it.

The remaining open questions I see are:

How to satisfy variants? variant="title" or TitleText? I proposed the former according to my mental model I attempted to describe above, heh.
What's the final recommendation for composing components with Text? TextWithIcon/IconWithText? What naming convention should be established for these, and how do we enforce that the Text docs page in storybook has all the text-related compositions?

david-crespo commented 3 years ago

The way it's written in the app is the API we are primarily concerned with here. We're talking about the ergonomics of calling the Text component.

I think we're mixing up two levels of API here.

The props API of a React component (the set of prop names and types) is distinct from the API used to create an element of that component (i.e., the way it's written in the app), of which there are several:

<Text size="sm" as="h3">
const Small = styled(Text).attrs({ size: 'sm', as: 'h3' }) followed by <Small>
const Small = styled.h3'${su.textSize('sm')}' followed by <Small>
React.createElement(Text, { size: 'sm', as: 'h3' }) (just for example, obviously we never do this)

These are all themselves interfaces to the underlying component API. I take this discussion to be about which of these APIs we prefer to use when adding text styles to something.

It seems we all agree that 1 is the nicest and that 2 is not so great. Unfortunately for us, we're using 2 75% of the time because 1 doesn't let us add other CSS. I propose that in those cases where we are currently using 2, we can use 3 instead. Syntax 3 gets us the same safety guarantees as 1 and 2 but without the syntax that mixes props and CSS and without typing issues of using as with extra props.

daegren commented 3 years ago

IMO, this conversation should be around the Component API (i.e. point 1) we want, regardless of what is already implemented. We should use learnings from our current implementations to inform our decisions about the component API as it's core to any other uses, as approaches 2 and 3 are just decorations of:

const Small: FC = ({children}) => 
  <Text size="sm" as="h3">{children}</Text>

Which is in turn decoration for:

const Small: FC = ({children}) =>
  React.createElement(Text, { size: 'sm', as: 'h3' }, children)

Given we're trying to figure out what's best given our use cases, and possible discussions of removing styled-components for emotion, both of those are really out of scope right now

david-crespo commented 3 years ago

I don't think it always needs to be a component at all (it can just be helpers that produce CSS), so the component API I want in those cases is no component API.

But if the mixin approach is not appealing to either of you under any circumstances, that's fine. We can revisit again later if adding more and more props to Text starts to get unwieldy. I'm surprised you want to use props to write CSS given your point of view on Tailwind.

<Text size="sm" color="green500" as="h3" uppercase>

<h3 tw="text-sm text-green-500 uppercase">

daegren commented 3 years ago

The whole point of a <Text> API is to hide away what is actually being used inside, and make it easier to change in the future. When building components, I feel it's better to know the text I'm using is design-system approved, I don't really care about the implementation, and frankly, having to remove tailwind (or something else) will be a full app change whereas changing the internals of an in-house <Text> component to reflect either hand-written CSS, or the next hot new framework, is super easy to do.

Going back to the original goals, which is where we need to get alignment and discussion on cause we obviously have a base disagreement with those.

Provide a quick way to know text is design system approved
Have a clear way of creating variants
Have a clear way of composing it with other Components
[NEW] Have a clear way to use the appropriate HTML tag where necessary
[NEW] Avoid cascading styles as they are prone to bugs, especially when components are composed.

I think the way styleUtils is designed is closer to what I was thinking as an alternative to the above, which is more around the idea of styling containers instead of leaf nodes, and relying on cascading to propagate those rules. I think cascading can lead to silly bugs and issues with styling, which is another goal I noted above. That being said, we can use styleUtils internally inside this component if need be, but that's also out of scope for this discussion.

Once we agree on these goals can we start thinking about what this API might look like

david-crespo commented 3 years ago

Closed by 29a49727da6a4c7158f532257686cda37a348114

oxidecomputer / console

Revisit Text API #117

API Goals

APIs

Text as Component

Other considerations

`children` as `ReactNode` or `string` vs `value` prop

Working with the wrapping `<span>` and `styled(Text)`

Caveats

Proposed CSS API

Variant API example

oxidecomputer / console

Revisit Text API #117

API Goals

APIs

Text as Component

Other considerations

children as ReactNode or string vs value prop

Working with the wrapping <span> and styled(Text)

Caveats

Proposed CSS API

Variant API example

`children` as `ReactNode` or `string` vs `value` prop

Working with the wrapping `<span>` and `styled(Text)`