Infra/ Incubator.WheelPicker selectedValue #1782
Conversation
| activeTextColor={Colors.primary} | ||
| inactiveTextColor={Colors.grey20} | ||
| items={monthItems} | ||
| textStyle={Typography.text60R} | ||
| selectedValue={selectedMonth} | ||
| /> | ||
|
|
||
| <Text h3>Years</Text> | ||
| <Text bodySmall grey30> | ||
| (Uncontrolled, initialValue passed) |
There was a problem hiding this comment.
Why do we need this example? Looks like the days example so we don't really need both now, no?
There was a problem hiding this comment.
I would add an example for changing 'initialValue' to show its effect, like a button that rolls the wheel by changing the prop's value.
There was a problem hiding this comment.
Not sure why we had the months example originally, maybe for string values.
Anyway, I've added two buttons for updating the years value with the initialValue.
| children, | ||
| items: propItems, | ||
| itemHeight, | ||
| preferredNumVisibleRows | ||
| }: PropTypes): Presenter => { | ||
| const value = !_.isUndefined(selectedValue) ? selectedValue : initialValue; | ||
| const value = initialValue; |
There was a problem hiding this comment.
seems like 'value' is unnecessary const now
| /** | ||
| * The current selected value | ||
| */ | ||
| selectedValue?: ItemProps | number | string; |
There was a problem hiding this comment.
This is a breaking change. I know we're in the incubator but I'm thinking that we might want to at least explain to the uses what's wrong. Maybe add a deprecated message or error them when we detect they passed this prop... or we rely on the ts errors alone?
There was a problem hiding this comment.
We don't have any internal users using this feature, regarding the community - we will add it under Breaking Changes in the release notes (and they also can decide not to update to this version until they fix it on their side)
There was a problem hiding this comment.
SectionsWheelPicker is using this component and it is not in the incubator. If someone passed 'selectedValue' to one of the sections (type WheelPickerProps) it will be affected by this change. How do we deal with that?
There was a problem hiding this comment.
You are right, but there's no internal use for SectionsWheelPicker in one app (we wrap it in private with different components), and for the community it's the same answer as before - we'll update it in the release notes, the ts we'll let them know as well, and they can stay with the previous version until they'll migrate to the other implementation.
There was a problem hiding this comment.
We usually don't introduce breaking changes in minor versions. Let's discuss this with @ethanshar
There was a problem hiding this comment.
It's ok, that's why we have the Incubator components.
We can merge this and as Lidor mention we'll add a breaking changes section for this (also mention the SectionWheelPicker component).
Moreover, this breaking change do offer an easy alternative and the community can also lock version till they handle the change.
lidord-wix
added
the
Important for Next Release
Description
Deprecate Incubator.WheelPicker
selectedValueprop in favor ofinitialValue(All the relevant changes are in the first commit, the second is only about formatting)
WOAUILIB-2308
Changelog
BREAKING CHANGE - Deprecate Incubator.WheelPicker
selectedValueprop in favor ofinitialValue