Comments (6)
- 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.
from helix-react.
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.
from helix-react.
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
orflavor
work better thanvariant
? - I've started the Best Practices wiki page to begin documenting results of conversations like this.
from helix-react.
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.
from helix-react.
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.
from helix-react.
Issue dated. Pursing updated solution.
from helix-react.
Related Issues (20)
- All components with handlers or methods should foward refs HOT 1
- Breadcrumbs HOT 1
- ChoiceTile HOT 1
- FileInput HOT 1
- Grid HOT 1
- HelixApp
- DescriptionList HOT 1
- Navigation HOT 1
- Pagination HOT 1
- SelectorStrip HOT 1
- Table HOT 1
- TextInput HOT 1
- TextArea HOT 1
- ButtonGroup HOT 1
- Box HOT 1
- Storybook + Web Components polyfills HOT 1
- Document React/Helix compatibility issues HOT 1
- Proposal: Component Refs and Typescript strict typing
- Implement Select Component Wrapper HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from helix-react.