adityamr15 / react-native-wheel-picker-expo

React Native wheel picker like iOS
MIT License
10 stars 8 forks source link

Implement haptics feedback #6

Closed gomo closed 2 years ago

gomo commented 2 years ago

https://github.com/adityamr15/react-native-wheel-picker-expo/issues/5#issuecomment-1178330300

Send a pull request for the ideas I submitted in the Issue.

Let me know about the Expo upgrade if you need it.

gomo commented 2 years ago

I just noticed that onScroll fires on initialSelectedIndex and the haptics happens first.

What about flagging it with onScrollBeginDrag, since it only needs to happen when the user scrolls?

I will try it.

adityamr15 commented 2 years ago

#5 (comment)

Send a pull request for the ideas I submitted in the Issue.

Let me know about the Expo upgrade if you need it.

I think we can go with SDK 43 as minimum requirement.

gomo commented 2 years ago

If we use the userScrolling flag, haptics will not occur when we tap the status bar.

Which do you think is better? Or do you have a better idea?

gomo commented 2 years ago

I think it is more natural for the library side to always generate haptics when haptics is true.

I prefer to leave the fine control to the developer who uses it.

I think the userScrolling flag should be eliminated.

adityamr15 commented 2 years ago

I just noticed that onScroll fires on initialSelectedIndex and the haptics happens first.

What about flagging it with onScrollBeginDrag, since it only needs to happen when the user scrolls?

I will try it.

I do agree the haptic should be disabled for initialSelectedIndex scroll we can set user flag for touched using onTouchStart instead of start draging/scroll

gomo commented 2 years ago

I think userScrolling flag still buggy, if I scroll the picker frequently.

You are right, sometimes the haptics does not occur.

I think it is because onMomentumScrollEnd fires later than onTouchStart.

I changed it so that each is flagged separately and haptics is turned on when one of them is true.

I think it is a bit redundant, but it does seem to work well.

gomo commented 2 years ago

https://github.com/adityamr15/react-native-wheel-picker-expo/pull/7#issuecomment-1179500747

in this case I think we can leave it to user/developer who use it. No need to implement it here.

I would like to change onMomentumScrollBegin and others so that we can add processing from the outside, something like this.

          onTouchStart={event => {
            this.userTouch = true

            if(flatListProps && flatListProps.onTouchStart){
              flatListProps.onTouchStart(event);
            }
          }}
          onTouchEnd={event => {
            this.userTouch = false

            if(flatListProps && flatListProps.onTouchEnd){
              flatListProps.onTouchEnd(event);
            }
          }}
          onMomentumScrollBegin={event => {
            this.momentumScrolling = true;

            if(flatListProps && flatListProps.onMomentumScrollBegin){
              flatListProps.onMomentumScrollBegin(event);
            }
          }}
          onMomentumScrollEnd={event => {
            this.momentumScrolling = false;

            if(flatListProps && flatListProps.onMomentumScrollEnd){
              flatListProps.onMomentumScrollEnd(event);
            }
          }}

Maybe onScroll could be added in the same way.

adityamr15 commented 2 years ago

Just take out momentumScrolling onMomentumScrollEnd onMomentumScrollBegin and onTouchEnd from code. Let user take the control by passing it to flatListProps.

Maybe onScroll could be added in the same way.

For now I don't think user can take control when scrolling, we should define the limitation of usage.

gomo commented 2 years ago

Just take out momentumScrolling onMomentumScrollEnd onMomentumScrollBegin and onTouchEnd from code. Let user take the control by passing it to flatListProps.

I move those codes above the {. .flatListProps}?

like...

        <FlatList
          keyExtractor={(_, index) => index.toString()}
          showsVerticalScrollIndicator={false}
          renderItem={(options) =>
            PickerItem(
              options,
              selectedIndex,
              {
                ...styles.listItem,
                backgroundColor: this.backgroundColor,
                fontSize: itemHeight / 2,
                height: itemHeight,
              },
              this.handleOnPressItem,
              this.props.renderItem as any
            )
          }
          onTouchStart={() => (this.userTouch = true)}
          onTouchEnd={() => (this.userTouch = false)}
          onMomentumScrollBegin={() => (this.momentumScrolling = true)}
          onMomentumScrollEnd={() => (this.momentumScrolling = false)}
          {...flatListProps}
          ref={this.flatListRef}
          initialScrollIndex={initialSelectedIndex}
          data={data}
          onScroll={(event) => {
            let index = Math.round(
              event.nativeEvent.contentOffset.y / itemHeight
            );
            this.handleOnSelect(index);
          }}
          getItemLayout={(_, index) => ({
            length: itemHeight,
            offset: index * itemHeight,
            index,
          })}
          snapToInterval={itemHeight}
        />

Is this okay?

gomo commented 2 years ago

Let user take the control by passing it to flatListProps.

I think I need a WheelPickerExpo ref to customize it, but when I try to use the ref in typescript, I get a warning.

const wheelRef = React.useRef<typeof WheelPickerExpo>();
...
<WheelPickerExpo
  ref={wheelRef} // here

...
(property) React.ClassAttributes<ViuPicker>.ref?: React.LegacyRef<ViuPicker> | undefined
No overload matches this call.
  Overload 1 of 2, '(props: IViuPickerProps | Readonly<IViuPickerProps>): ViuPicker', gave the following error.
    Type 'MutableRefObject<typeof ViuPicker | undefined>' is not assignable to type 'LegacyRef<ViuPicker> | undefined'.
      Type 'MutableRefObject<typeof ViuPicker | undefined>' is not assignable to type 'RefObject<ViuPicker>'.
        Types of property 'current' are incompatible.
          Type 'typeof ViuPicker | undefined' is not assignable to type 'ViuPicker | null'.
            Type 'undefined' is not assignable to type 'ViuPicker | null'.
  Overload 2 of 2, '(props: IViuPickerProps, context: any): ViuPicker', gave the following error.
    Type 'MutableRefObject<typeof ViuPicker | undefined>' is not assignable to type 'LegacyRef<ViuPicker> | undefined'.ts(2769)
index.d.ts(136, 9): The expected type comes from property 'ref' which is declared here on type 'IntrinsicAttributes & IntrinsicClassAttributes<ViuPicker> & Pick<Readonly<IViuPickerProps> & Readonly<...>, "onChange" | ... 6 more ... | "children"> & Partial<...> & Partial<...>'
index.d.ts(136, 9): The expected type comes from property 'ref' which is declared here on type 'IntrinsicAttributes & IntrinsicClassAttributes<ViuPicker> & Pick<Readonly<IViuPickerProps> & Readonly<...>, "onChange" | ... 6 more ... | "children"> & Partial<...> & Partial<...>'

I'm not familiar with typescript and release-it, so I may be wrong, but I think we have to export ViuPicker too, or change the class name to WheelPickerExpo to get the correct ref.

If there is a way to do this, please let me know.

adityamr15 commented 2 years ago

If there is a way to do this, please let me know.

Did you try with forwardRef? If still not working I think we should rename the component than we export directly, but will be effortless if we implement with forwardRef

gomo commented 2 years ago

Did you try with forwardRef?

I'm not quite sure about using useRef, but I think we don't have access to the type information unless export the class directly.

class ViuPicker extends Component<IViuPickerProps, IViuPickerState> {
...
}
const WheelPickerExpo = ViuPicker;
export default WheelPickerExpo;

When assigned to a variable, typescript determines that it is a value, not a class, so we do not have access to the type information.

Screen Shot 2022-07-22 at 13 50 10

Even with typeof, we cannot access the correct information because it is the type of the class.

Screen Shot 2022-07-22 at 13 35 24

When exporting the class directly, we can see all the properties and methods.

class WheelPickerExpo extends Component<IViuPickerProps, IViuPickerState> {
...
}

export default WheelPickerExpo;
Screen Shot 2022-07-22 at 13 36 06

I made the change because I didn't think it would be a problem to change to directly export the class. Let me know if you have any problems.

adityamr15 commented 2 years ago

ok thank you, will merge this PR