Text, Text Area, Switch, Pill #106
Conversation
- Using `container` prefix to describe the outer most tag. IE: containId, or containsClass
|
Deploy preview for helix-react ready! Built with commit 4e3639a |
After seeing developers interact with these components, it seems like the more intuitive approuch is to expect the className goes on the parent most element. Putting class names wrapping element to improve developer experience.
100stacks
left a comment
100stacks
left a comment
There was a problem hiding this comment.
Please see comment below. Most are just for my understanding. Thanks.
| required: PropTypes.bool, | ||
| label: PropTypes.bool, | ||
| name: PropTypes.string, | ||
| onChange: PropTypes.func, |
There was a problem hiding this comment.
I've noticed this on a few components, just wanted to verify that onChange is optional.
There was a problem hiding this comment.
yep onChange is optional (in my mind). One can actually use any handler, onKeyUp, OnKeyDown, etc... since we pass them all on the the component etc .I would consider them all optional, but most likely a developer would want to use one of them, unless they are using a traditional form submissions (without REST call), i'm sure those use cases still exist as rare as they are.
There was a problem hiding this comment.
all the different handlers could all live in the propTypes though I do wonder once we switch over to typeScript we will also not need to ship all the propType stuff in the bundles. if your ok with it, lets handle any major changes to event handler propTypes in another PR.
There was a problem hiding this comment.
I agree with you @nicko-winner. I don't think we'll have TypeScript support for alpha, so I'm all for holding off.
There was a problem hiding this comment.
I'm all for getting an alpha release out sooner, then we can iterate as needed.
- Switch updates - Proptypes updates
100stacks
left a comment
100stacks
left a comment
There was a problem hiding this comment.
AWESOME WORK!!! 🎉 👍
Dev LGTM
Usingcontainerprefix to describe the outer most tag. IE: containId, or containsClass, decided against taking the className going to the input, as maybe some one one might want to apply a class directly to the input.^After seeing developers interact with these components, it seems like the more intuitive approach is to expect the className goes on the parent most element. Putting class names wrapping element to improve developer experience.
I decided against adding help & error props to inputs. I was missing some helix styles for help. Rather than invent the wheel I think I will wait until helix forms comes out.