Allow stepper to be used as a controlled component #688
Conversation
mking-clari
force-pushed
the
stepper-controlled-component
branch
from
18fce36 to
c7b5530
Compare
| @@ -92,6 +97,10 @@ export default class ArrowKeyStepper extends PureComponent { | |||
| ) | |||
| } | |||
|
|
|||
| _isComponentState () { | |||
| return this._isComponentState() ? this.state : this.props | ||
| } | ||
|
|
||
| _setRelevantState ({ scrollToColumn, scrollToRow }) { |
| @@ -147,4 +156,16 @@ export default class ArrowKeyStepper extends PureComponent { | |||
| this._rowStartIndex = rowStartIndex | |||
| this._rowStopIndex = rowStopIndex | |||
| } | |||
|
|
|||
| _getRelevantState () { | |||
yarn.lock
Outdated
| version "1.3.1" | ||
| resolved "https://registry.yarnpkg.com/loose-envify/-/loose-envify-1.3.1.tgz#d1a8ad33fa9ce0e713d65fdd0ac8b748d478c848" | ||
| dependencies: | ||
| js-tokens "^3.0.0" | ||
|
|
||
| loose-envify@^1.3.0: |
There was a problem hiding this comment.
Probably fine, but can we drop the yarn.lock changes from this PR since it's not actually introducing any new deps?
| @@ -14,11 +14,12 @@ export default class ArrowKeyStepper extends PureComponent { | |||
| }; | |||
|
|
|||
| static propTypes = { | |||
| children: PropTypes.func.isRequired, | |||
| children: PropTypes.oneOfType([PropTypes.func, PropTypes.node]).isRequired, | |||
There was a problem hiding this comment.
This change seems wrong. If you pass a node, wouldn't things break now since the render function expect children to always be a function?
| className: PropTypes.string, | ||
| columnCount: PropTypes.number.isRequired, | ||
| disabled: PropTypes.bool.isRequired, | ||
| mode: PropTypes.oneOf(['cells', 'edges']), | ||
| onScrollToChange: PropTypes.func, |
There was a problem hiding this comment.
Seems like it's at least feasible that someone might want to observe scroll changes without being in controlled mode. I wonder if controlled mode should be a more explicit flag (eg a isControlled boolean prop or something)?
mking-clari
force-pushed
the
stepper-controlled-component
branch
3 times, most recently
from
c02da77 to
af427c8
Compare
mking-clari
force-pushed
the
stepper-controlled-component
branch
from
af427c8 to
bc74522
Compare
|
@bvaughn Thanks for your feedback. Updated per your review. note: I removed |
|
I just published this feature to NPM as Assuming no problems are reported with the RC, this feature will go out with 9.8.0 sometime this weekend. 😁 |
|
Haven't heard anything negative about the RC so I just released 9.8.0 |
Implements #687
ArrowKeyStepperas a controlled componentArrowKeyStepper's state).ArrowKeyStepperas a controlled component:onScrollToChange({ scrollToColumn, scrollToRow })as a propscrollToColumnas a propscrollToRowas a propArrowKeyStepperis being used as a controlled component, it does not callsetStatefor performance reasons (instead it uses the props directly).PR checklist
onScrollToChangeis called when key down occursonScrollToChangeis not called when props are updated (if the change occurs via props, the source of truth already knows about the change)Enable click selection. I have enabled this setting by default in the demo because I think users normally want to be able to click to select.Let me know your feedback. I will be happy to make changes.