-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor/component-types #212
Conversation
@@ -87,8 +91,10 @@ const Expander = React.createClass(createLucidComponentDefinition({ | |||
}, | |||
|
|||
componentWillReceiveProps(nextProps) { | |||
const currentLabel = _.first(Expander.Label.findInAllAsProps(this.props)).children; | |||
const nextLabel = _.first(Expander.Label.findInAllAsProps(nextProps)).children; | |||
//const currentLabel = _.first(Expander.Label.findInAllAsProps(this.props)).children; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
childProps: { | ||
Button: { ...Button.propTypes } | ||
components: { | ||
Button: createClass({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you not just use a reference to the Button
component here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
er, i guess not since it has a render fn... this part seems a little weird
seems like it would be good to be able to get at least propTypes
off of Button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right we should reference it as is right here instead of creating a new type, but the goal of this change was a 1:1 change in how child components are defined and filtered, we should have another PR that cleans up any redundancies that we identify with child components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't it be weird since Button
has a render function, tho
I'm seeing an issue with the debounced |
- There are inherent collisions/weirdness when we pass an `onChange` from the reducer but the consumer is trying to manually handle `onChangeDebounced`.
Conflicts: src/components/CheckboxLabeled/CheckboxLabeled.jsx src/components/RadioButtonLabeled/RadioButtonLabeled.jsx src/components/RadioGroup/RadioGroup.spec.jsx src/components/SwitchLabeled/SwitchLabeled.jsx src/components/TextFieldValidated/TextFieldValidated.jsx src/index.js
lost in merge conflicts
Refactor
createClass
to be used for both normal and child components, and simplify the util functions for finding/filtering for child components.PR Checklist
ChromeFirefoxSafariIE11 (Win 7)Edge (Win 10)semver-
labelsOne core team UX approval