Skip to content

Infra/ Incubator.WheelPicker selectedValue #1782

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 25, 2022
Merged

Infra/ Incubator.WheelPicker selectedValue #1782

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
768c78f
deprecate Incubator.WheelPicker selectedValue in favor of initialValue
lidord-wix Jan 17, 2022
9fc9f36
formatting
lidord-wix Jan 17, 2022
6a01899
update screen
lidord-wix Jan 18, 2022
843c2fa
remove redundant const
lidord-wix Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 4 additions & 51 deletions demo/src/screens/incubatorScreens/WheelPickerScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import React, {useCallback, useState} from 'react';
import {View, Text, Incubator, Colors, Typography, Button, Dialog} from 'react-native-ui-lib';
import _ from 'lodash';

type WheelPickerValue = Incubator.WheelPickerProps['initialValue'];

const monthItems = _.map([
'January',
'February',
Expand All @@ -25,10 +23,7 @@ const yearItems = _.times(2030, i => i)
.map(item => ({label: `${item}`, value: item}));
const dayItems = _.times(31, i => i + 1).map(day => ({label: `${day}`, value: day}));

const useData = (initialMonth?: string, initialYear?: string, initialDays?: number) => {
const [selectedMonth, setMonth] = useState<WheelPickerValue>(initialMonth);
const [, setYear] = useState<WheelPickerValue>(initialYear);
const [selectedDays, setDays] = useState<WheelPickerValue>(initialDays);
export default () => {
const [showDialog, setShowDialog] = useState(false);

const onPickDaysPress = useCallback(() => {
Expand All @@ -39,75 +34,33 @@ const useData = (initialMonth?: string, initialYear?: string, initialDays?: numb
setShowDialog(false);
}, []);

const onMonthChange = useCallback((item: WheelPickerValue, _: number) => {
setMonth(item);
}, []);

const onYearChange = useCallback((item: WheelPickerValue, _: number) => {
setYear(item);
}, []);

const onDaysChange = useCallback((item: WheelPickerValue, _: number) => {
setDays(item);
}, []);

return {
onMonthChange,
onYearChange,
onDaysChange,
selectedMonth,
selectedDays,
onPickDaysPress,
onDialogDismissed,
showDialog
};
};

export default () => {
const {
selectedMonth,
onMonthChange,
onYearChange,
selectedDays,
onDaysChange,
onPickDaysPress,
onDialogDismissed,
showDialog
} = useData('February', undefined, 5);

return (
<View flex padding-page>
<Text h1>Wheel Picker</Text>

<View marginT-s5 centerH>
<Text h3>Months</Text>
<Incubator.WheelPicker
onChange={onMonthChange}
initialValue={'February'}
activeTextColor={Colors.primary}
inactiveTextColor={Colors.grey20}
items={monthItems}
textStyle={Typography.text60R}
selectedValue={selectedMonth}
/>

<Text h3>Years</Text>
<Text bodySmall grey30>
(Uncontrolled, initialValue passed)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this example? Looks like the days example so we don't really need both now, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</Text>
<View width={'100%'} marginT-s3>
<Incubator.WheelPicker
onChange={onYearChange}
numberOfVisibleRows={3}
initialValue={2021}
items={yearItems}
/>
<Incubator.WheelPicker numberOfVisibleRows={3} initialValue={2022} items={yearItems}/>
</View>
</View>

<View marginB-s10>
<Button marginT-40 label={'Pick Days'} marginH-100 onPress={onPickDaysPress}/>
<Dialog width={'90%'} height={260} bottom visible={showDialog} onDismiss={onDialogDismissed}>
<Incubator.WheelPicker onChange={onDaysChange} selectedValue={selectedDays} label={'Days'} items={dayItems}/>
<Incubator.WheelPicker initialValue={5} label={'Days'} items={dayItems}/>
</Dialog>
</View>
</View>
Expand Down
8 changes: 2 additions & 6 deletions generatedTypes/src/incubator/WheelPicker/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,9 @@ export declare enum WheelPickerAlign {
}
export interface WheelPickerProps {
/**
* Initial value (doesn't work with selectedValue)
* Initial value
*/
initialValue?: ItemProps | number | string;
/**
* The current selected value
*/
selectedValue?: ItemProps | number | string;
/**
* Data source for WheelPicker
*/
Expand Down Expand Up @@ -75,7 +71,7 @@ export interface WheelPickerProps {
declare const _default: React.ComponentClass<WheelPickerProps & {
useCustomTheme?: boolean | undefined;
}, any> & {
({ items: propItems, itemHeight, numberOfVisibleRows, activeTextColor, inactiveTextColor, textStyle, label, labelStyle, labelProps, onChange, align, style, children, initialValue, selectedValue, testID }: WheelPickerProps): JSX.Element;
({ items: propItems, itemHeight, numberOfVisibleRows, activeTextColor, inactiveTextColor, textStyle, label, labelStyle, labelProps, onChange, align, style, children, initialValue, testID }: WheelPickerProps): JSX.Element;
alignments: typeof WheelPickerAlign;
displayName: string;
};
Expand Down
4 changes: 1 addition & 3 deletions generatedTypes/src/incubator/WheelPicker/usePresenter.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { ItemProps } from './Item';
export declare type ItemValueTypes = ItemProps | number | string;
declare type PropTypes = {
initialValue?: ItemValueTypes;
selectedValue?: ItemValueTypes;
children?: JSX.Element | JSX.Element[];
items?: ItemProps[];
itemHeight: number;
Expand All @@ -15,10 +14,9 @@ declare type RowItem = {
};
interface Presenter {
items: ItemProps[];
shouldControlComponent: (offset: number) => boolean;
index: number;
height: number;
getRowItemAtOffset: (offset: number) => RowItem;
}
declare const usePresenter: ({ initialValue, selectedValue, children, items: propItems, itemHeight, preferredNumVisibleRows }: PropTypes) => Presenter;
declare const usePresenter: ({ initialValue, children, items: propItems, itemHeight, preferredNumVisibleRows }: PropTypes) => Presenter;
export default usePresenter;
51 changes: 20 additions & 31 deletions src/incubator/WheelPicker/__tests__/usePresenter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@ import usePresenter from '../usePresenter';
import {renderHook} from '@testing-library/react-hooks';

describe('WheelPicker presenter tests', () => {

const makeSUT = ({selectedValue, items = makeItems(9), children, itemHeight = 10, preferredNumVisibleRows = 20}) => {
const makeSUT = ({items = makeItems(9), children, initialValue, itemHeight = 10, preferredNumVisibleRows = 20}) => {
return renderHook(() =>
usePresenter({
selectedValue,
items,
children,
initialValue,
itemHeight,
preferredNumVisibleRows
}));
Expand All @@ -17,7 +16,7 @@ describe('WheelPicker presenter tests', () => {
const makeItems = (count, stringValue) => {
const items = [];
while (count >= items.length) {
const someData = stringValue ? (stringValue + items.length) : items.length;
const someData = stringValue ? stringValue + items.length : items.length;
const item = {value: someData, label: someData};
items.push(item);
}
Expand All @@ -27,68 +26,58 @@ describe('WheelPicker presenter tests', () => {
it('expect height of the content-view to be itemHeight * preferredNumVisibleRows', () => {
let sut = makeSUT({items: makeItems(44), itemHeight: 10, preferredNumVisibleRows: 5});
expect(sut.result.current.height).toEqual(50);

sut = makeSUT({items: makeItems(10), itemHeight: 20, preferredNumVisibleRows: 3});
expect(sut.result.current.height).toEqual(60);

sut = makeSUT({items: makeItems(10), itemHeight: 0, preferredNumVisibleRows: 0});
expect(sut.result.current.height).toEqual(0);
});

it('Expect to find items by their string types', () => {
let sut = makeSUT({items: makeItems(15, 'a'), selectedValue: 'a2'});
let sut = makeSUT({items: makeItems(15, 'a'), initialValue: 'a2'});
expect(sut.result.current.index).toEqual(2);

sut = makeSUT({items: makeItems(100, 'bbb'), selectedValue: 'bbb71'});
sut = makeSUT({items: makeItems(100, 'bbb'), initialValue: 'bbb71'});
expect(sut.result.current.index).toEqual(71);

// no data found
sut = makeSUT({items: makeItems(10, 'b'), selectedValue: '$$$'});
sut = makeSUT({items: makeItems(10, 'b'), initialValue: '$$$'});
expect(sut.result.current.index).toEqual(-1);
});

it('Expect to find items by their number types', () => {
let sut = makeSUT({items: makeItems(11), selectedValue: 0});
let sut = makeSUT({items: makeItems(11), initialValue: 0});
expect(sut.result.current.index).toEqual(0);

sut = makeSUT({items: makeItems(8), selectedValue: 4});
sut = makeSUT({items: makeItems(8), initialValue: 4});
expect(sut.result.current.index).toEqual(4);

sut = makeSUT({items: makeItems(18), selectedValue: 18});
sut = makeSUT({items: makeItems(18), initialValue: 18});
expect(sut.result.current.index).toEqual(18);
sut = makeSUT({items: makeItems(18), selectedValue: 99});

sut = makeSUT({items: makeItems(18), initialValue: 99});
expect(sut.result.current.index).toEqual(-1);

sut = makeSUT({items: makeItems(0), selectedValue: 99});
sut = makeSUT({items: makeItems(0), initialValue: 99});
expect(sut.result.current.index).toEqual(-1);
sut = makeSUT({items: makeItems(0), selectedValue: 0});

sut = makeSUT({items: makeItems(0), initialValue: 0});
expect(sut.result.current.index).toEqual(0);
});

it('Expect to find items by their object of {value, label} types', () => {
const {result} = makeSUT({items: makeItems(15, 'b'), selectedValue: {value: 'b6', label: 'abc'}});
const {result} = makeSUT({items: makeItems(15, 'b'), initialValue: {value: 'b6', label: 'abc'}});
expect(result.current.index).toEqual(6);
});

it('Expect component to be controlled and not change by offset', () => {
const {result} = makeSUT({selectedValue: 2, itemHeight: 100});

let offset = 300;
expect(result.current.shouldControlComponent(offset)).toEqual(true);

offset = 200;
expect(result.current.shouldControlComponent(offset)).toEqual(false);
});

it('Expect getRowItemAtOffset to return the right row for offset', () => {
let sut = makeSUT({selectedValue: 2, itemHeight: 100});
let sut = makeSUT({initialValue: 2, itemHeight: 100});

let offset = 300;
expect(sut.result.current.getRowItemAtOffset(offset).value).toEqual(3);

sut = makeSUT({selectedValue: 0, itemHeight: 100});
sut = makeSUT({initialValue: 0, itemHeight: 100});
offset = 0;
expect(sut.result.current.getRowItemAtOffset(offset).value).toEqual(0);
});
Expand Down
38 changes: 14 additions & 24 deletions src/incubator/WheelPicker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,9 @@ export enum WheelPickerAlign {

export interface WheelPickerProps {
/**
* Initial value (doesn't work with selectedValue)
* Initial value
*/
initialValue?: ItemProps | number | string;
/**
* The current selected value
*/
selectedValue?: ItemProps | number | string;
Copy link
Collaborator

@Inbal-Tish Inbal-Tish Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Inbal-Tish Inbal-Tish Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually don't introduce breaking changes in minor versions. Let's discuss this with @ethanshar

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* Data source for WheelPicker
*/
Expand Down Expand Up @@ -99,8 +95,7 @@ const WheelPicker = ({
align = WheelPickerAlign.CENTER,
style,
children,
initialValue,
selectedValue,
initialValue = 0,
testID
}: WheelPickerProps) => {
const scrollView = useRef<Animated.ScrollView>();
Expand All @@ -112,12 +107,10 @@ const WheelPicker = ({
const {
height,
items,
shouldControlComponent,
index: currentIndex,
getRowItemAtOffset
} = usePresenter({
initialValue,
selectedValue,
items: propItems,
children,
itemHeight,
Expand All @@ -126,17 +119,9 @@ const WheelPicker = ({

const prevInitialValue = useRef(initialValue);
const prevIndex = useRef(currentIndex);
const [scrollOffset, setScrollOffset] = useState(currentIndex * itemHeight);
const [flatListWidth, setFlatListWidth] = useState(0);
const keyExtractor = useCallback((item: ItemProps, index: number) => `${item}.${index}`, []);

useEffect(() => {
// This effect enforce the index to be controlled by selectedValue passed by the user
if (shouldControlComponent(scrollOffset)) {
scrollToIndex(currentIndex, true);
}
});

useEffect(() => {
// This effect making sure to reset index if initialValue has changed
!isUndefined(initialValue) && scrollToIndex(currentIndex, true);
Expand All @@ -149,13 +134,14 @@ const WheelPicker = ({
} else {
onChange?.(value, index);
}
}, [initialValue, onChange]);
},
[initialValue, onChange]);

const onValueChange = useCallback((event: NativeSyntheticEvent<NativeScrollEvent>) => {
setScrollOffset(event.nativeEvent.contentOffset.y);
const {index, value} = getRowItemAtOffset(event.nativeEvent.contentOffset.y);
_onChange(value, index);
}, [_onChange, getRowItemAtOffset]);
},
[_onChange, getRowItemAtOffset]);

const onMomentumScrollEndAndroid = (index: number) => {
// handle Android bug: ScrollView does not call 'onMomentumScrollEnd' when scrolled programmatically (https://github.com/facebook/react-native/issues/26661)
Expand Down Expand Up @@ -188,7 +174,8 @@ const WheelPicker = ({

const selectItem = useCallback(index => {
scrollToIndex(index, true);
}, [itemHeight]);
},
[itemHeight]);

const renderItem = useCallback(({item, index}) => {
return (
Expand All @@ -208,11 +195,13 @@ const WheelPicker = ({
testID={`${testID}.item_${index}`}
/>
);
}, [itemHeight]);
},
[itemHeight]);

const getItemLayout = useCallback((_data, index: number) => {
return {length: itemHeight, offset: itemHeight * index, index};
}, [itemHeight]);
},
[itemHeight]);

const updateFlatListWidth = useCallback((width: number) => {
setFlatListWidth(width);
Expand Down Expand Up @@ -254,7 +243,8 @@ const WheelPicker = ({

const fader = useMemo(() => (position: FaderPosition) => {
return <Fader visible position={position} size={60}/>;
}, []);
},
[]);

const separators = useMemo(() => {
return (
Expand Down
Loading