Make Slider a controlled component #1426
Conversation
Now sets the value attribute from props.value.
interactivellama
changed the title
…-system-react into DanFerro-slider-value-fix
Make onChange callback provide a number, since a number is passed into value.
Make onChange callback provide a number, since a number is passed into value.
…react into DanFerro-slider-value-fix # Conflicts: # components/slider/index.jsx
…lador/design-system-react into DanFerro-slider-value-fix
|
@garygong This was the question I was asking about earlier that was whether a change to a component was a breaking change if the component did not work as intended in the first place. |
interactivellama
requested review from
futuremint
and removed request for
garygong
|
Moving Review request to @futuremint if he is available this week. |
|
I'm available and I'll review this today. |
components/slider/index.jsx
Outdated
| @@ -140,13 +139,13 @@ class Slider extends React.Component { | |||
|
|
|||
| handleChange = (event) => { | |||
| if (isFunction(this.props.onChange)) { | |||
| this.props.onChange(event, { value: event.target.value }); | |||
| this.props.onChange(event, { value: parseInt(event.target.value, 10) }); | |||
There was a problem hiding this comment.
This could pose a problem should a user want to make a slider for floats (a slider between 0 and 1 for example). I'm not sure we want to have parseInt here because we can't assume.
There was a problem hiding this comment.
The input returns a text value, but we expect a number be passed in. I'll look into using Number().
| @@ -400,7 +400,7 @@ const Dialog = createReactClass({ | |||
| } | |||
|
|
|||
| if (this.props.style) { | |||
| style = Object.assign(style, this.props.style); | |||
| style = { ...style, ...this.props.style }; | |||
utilities/warning/url-exists.js
Outdated
| /* eslint-enable max-len */ | ||
| hasWarned[`${control}-path`] = !!url; | ||
| } | ||
| const warn = (control, url, comment) => (res) => { |
There was a problem hiding this comment.
Is there a reason to add support for fetch here? Does this address a maintenance issue in the backlog or something else?
There was a problem hiding this comment.
Sorry, that's already in master. I'll remove it.
…to DanFerro-slider-value-fix
instead of parseInt in order to allow decimal values
|
Now sets the value attribute from props.value.
Fixes #1398
Pull Request Review checklist (do not remove)
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.