[IDEA] "appearance" prop

[CITguy]

Currently Buttons accept two separate props to modify it's size (small, medium, large) and type (primary, secondary, tertiary). Evergreen buttons use an appearance prop to tweak visual styles. So I started to wonder if it would be possible to use a single prop across HelixReact components to modify appearance without the need to directly apply CSS classes.

Current

<Button/>
<Button size="large" />
<Button variant="primary" />
<Button size="large" variant="primary" />

<button class="hxBtn"></button>
<button class="hxBtn hxLg"></button>
<button class="hxBtn hxPrimary"></button>
<button class="hxBtn hxLg hxPrimary"></button>

Concept

• medium and secondary are ignored (default size and type)

<Button/>
<Button appearance="small primary" />
<Button appearance="medium tertiary" />
<Button appearance="large secondary" />

<button class="hxBtn"></button>
<button class="hxBtn hxSm hxPrimary"></button>
<button class="hxBtn hxTertiary"></button>
<button class="hxBtn hxLg"></button>

Pros and Cons

• PROS

  • single prop
  • purpose of appearance prop easily understandable

• CONS

  • may require more JavaScript to translate to CSS classes
  • appearance prop could be something shorter to type

[vlutton]

• CONS
  • If you use typescript this will be tougher to strongly type i.e.:

interface ButtonProps {
  size: 'small' | 'large' | 'medium'
  variant: 'primary' | 'secondary' | 'tertiary'
}

during implementation you get the list of what is available to be entered in the props vs the somewhat generic and generalized strings that show up in appearance (forcing you to leave the editor and go evaluate the documentation to see what you can type into this string). I BELIEVE this would also be the case with flow but I am not as familiar with that.

[eddywashere]

I think brevity and consistency go a long way. A single prop might be too extreme as it introduces a lot of logic into a single entry point. The only single prop that should do that is, className.

re: Buttons accept two separate props to modify it's size, does it really? And should it? If variant changes the button size that would be kind of hectic as a consumer to make sure the content around the button stays aligned properly.

Here's a list of prop type standards that went a long way for making reactstrap simple to use because I think it followed a basic mental model of how the props should work: reactstrap/reactstrap#2

Current PropType “standards":

isOpen - current state for items like dropdown, popover, tooltip
toggle - callback for toggling isOpen in the controlling component
color - applies color classes, ex: <Button color="danger"/> // <button class="btn btn-danger"/>
size for controlling size classes. ex: <Button size="sm"/> // <button class="btn btn-sm"/>
boolean based props (attributes) when possible for alt classes or sr-only content

The only big feedback I had was that isOpen probably wasn't generic enough. If I could go back in time I would have just called it, isToggled. That would have covered more components.

Lastly, I think variant is a weird prop, let alone word describe something that ultimately affects the colors rendered in the component. I think color is easier to communicate.

[CITguy]

Thank you for your feedback!

You've both brought up good points.

TypeScript

@vlutton: I'm just starting to learn more about TypeScript, so this is useful information.

Size vs Variant

@eddywashere:
Buttons don't have two props to change just the size (parens broke the sentence flow in my comment). What I meant for it to say was Buttons accept a size prop to change the size/height of a button and a variance prop to modify the visual style between Primary, Secondary, and Tertiary buttons styles.

Size

CSS styles that change the size modify both vertical and horizontal spacing within a button.

Variant

In addition to colors, variant styles modify horizontal spacing within a button (retaining size/height). I agree that the term "variant" probably isn't the best prop name, but I'm not sure color accurately reflects modifying the visual style, mainly because each button variant could potentially have different colors (e.g., a primary success button, secondary warning button, tertiary info button, etc.)

What about something like kind or flavor?

Predicate Properties

I like the idea of using is* for predicate properties (isOpen, isPaused, etc.). I'm worried that using isToggled might get confusing down the line for components that could be "toggled" in more ways than one (I can't think of any specific examples right now). I'd recommend reserving isToggled for toggle buttons (e.g., pagination buttons to denote current page selection).

Summary

• I agree that we should avoid overloading props.
• I like the idea of is* predicate properties (isOpen, isPaused, etc.), where they make sense.
• Would a prop named kind or flavor work better than variant?
• I've started the Best Practices wiki page to begin documenting results of conversations like this.

[CITguy]

FYI: I just realized that it would be additional work to use is* predicates for boolean props that are supported by vanilla HTML elements (disabled, hidden, checked, open, muted, etc.) and/or supported by HelixUI elements (static, invalid, paused, etc.).

Example

Say we had a button that can be disabled. Vanilla <button> elements support the disabled attribute.

isDisabled (non-optimal)

If we supported isDisabled, we'd have to figure out how to keep it from conflicting with the disabled prop.

SomeComponent.jsx

return (<Button isDisabled={true}>Disabled Button</Button>);

Button.jsx

render () {
  const { disabled, isDisabled, ...passThru } = this.props;
  return (<button disabled={disabled||isDisabled} {...passthru}></button>);
}

disabled (better)

SomeComponent.jsx

return (<Button disabled={true}>Disabled Button</Button>);

Button.jsx

render () {
  return (<button {...this.props}></button>);
}

Summary

I think we might still be able to use is* predicate properties, but we'd have to evaluate them on a case-by-case basis to make sure it wouldn't conflict with another boolean prop.

[eddywashere]

boolean based props (attributes) when possible for alt classes or sr-only content

Yes, “when possible”. For the disabled example that’s exactly what reactstrap does.

It was easier to have a whitelist of html attributes and their react variants (for -> htmlFor). It’s weird with web components since they can be anything. Which is why I mentioned that web components are not just basic html/css. It’s like a separate thing that you have to keep in mind. Another layer. It’s an add on to dom apis but the bottom line is it’s a thing in and of itself.

^ Just clarifying the position I had because I had this context in mind and it was difficult to communicate until you’re in the thick of it.

[100stacks]

Issue dated. Pursing updated solution.