Implement PickerAvoidingView and PickerStateProvider

[cubuspl42]

...which provide tools for ensuring that a picker is not covered by its own modal on iOS.

I'm still testing these changes, but they are ready to be reviewed, as I don't expect any major changes in the approach for now.

[cubuspl42]

What's still to be done: creating a symmetric PR to @react-native-picker/picker and adding a relevant comment that these changes are subject to being reverted if this functionality is merged to the upstream.

[0xmiros]

@Julesssss please assign yourself as a reviewer

[0xmiros]

index.ios.js:

    return (
        <View style={StyleSheet.compose(props.style, {paddingBottom: shouldAddSpace ? IOS_MODAL_HEIGHT : 0})}>
            {props.children}
        </View>
    );

index.js:

const PickerAvoidingView = (props) => {
    const viewProps = _.omit(props, ['enabled']);
    return (
        // eslint-disable-next-line react/jsx-props-no-spreading
        <View {...viewProps} />
    );
};

@cubuspl42 this is my suggestion

[0xmiros]

If we animate like KeyboardAvoidingView, that would be a great bonus point.
So IOS_MODAL_HEIGHT value animates with same value and same duration as picker modal.
cc: @Julesssss

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-04-13.at.12.55.59.mp4

[cubuspl42]

If we animate like KeyboardAvoidingView, that would be a great bonus point. So IOS_MODAL_HEIGHT value animates with same value and same duration as picker modal. cc: @Julesssss

Please note that the problem is of a bit different nature. Resizing the height of the bottom space does not scroll or move the content inside the ScrollView. What makes the picker element move over the modal is the scrollTo call.

[0xmiros]

If we animate like KeyboardAvoidingView, that would be a great bonus point. So IOS_MODAL_HEIGHT value animates with same value and same duration as picker modal. cc: @Julesssss

Please note that the problem is of a bit different nature. Resizing the height of the bottom space does not scroll or move the content inside the ScrollView. What makes the picker element move over the modal is the scrollTo call.

I think you didn't get my point. It's not related to scroll.
My video is about resizing height, not scrolling to some view. Button is animating not because of scroll but resizing.
Scrolling should happen only after animation complete.

[0xmiros]

@cubuspl42 I opened slack discussion to move things faster. Let's discuss there for further comments https://expensify.slack.com/archives/C01GTK53T8Q/p1681386144253199

[0xmiros]

@cubuspl42 please fix conflict and point correct hash on app PR

[0xmiros]

Is there any other way to avoid this wrapper?

[cubuspl42]

Hooks can't be used in class components. I used useEffect hook, indirectly via a helper functional component, to ensure that no effect is run after our component gets unmounted. Do you think this should be done manually in the component lifecycle methods?

[0xmiros]

Do you think this should be done manually in the component lifecycle methods?

yes, we should try to avoid complex code as possible

[cubuspl42]

useEffect logic, implemented using component lifecycle methods, looks more or less like this:

componentDidMount() {
    this.performEffect();
  }

  componentDidUpdate(prevProps) {
    // check if dependencies changed
    if (/* dependencies changed */) {
      // Clean up the previous effect before applying the new one
      if (this.cleanupEffect) {
        this.cleanupEffect();
      }
      this.performEffect();
    }
  }

  componentWillUnmount() {
    if (this.cleanupEffect) {
      this.cleanupEffect();
    }
  }

  performEffect() {
    // effect code here

    // store the cleanup function as an instance variable
    this.cleanupEffect = () => {
      // cleanup/cancelling code here
    };
  }

Is that the simple version?

[cubuspl42]

Hm, maybe I'm missing some simpler options. I do agree that things should be as simple as possible. Would you share what is on your mind? You can see the problem we're solving. How would you approach that?

[0xmiros]

did you already try onModalShow callback?

[cubuspl42]

By "there's no need for schedule", you're saying that you wouldn't cancel this timeout in any circumstances, right? So you would guess that it wouldn't cause any trouble if it was run after the picker component is unmounted, right?

I'm not against it, I'm just trying to ensure that I read your words as you mean them. I haven't even tested such edge case, as it also wouldn't be very easy to test. It's just a good pattern to cancel all cancellable side effects when the component which "owns" them is unmounted, destroyed, or something like that.

[0xmiros]

you wouldn't cancel this timeout in any circumstances

I asked: "Isn't that enough to clearTimout on componentWillUnmount?"

Also did you try onModalShow callback?

[cubuspl42]

I asked: "Isn't that enough to clearTimout on componentWillUnmount?"

Sorry, I missed that part. It's not really the same, as useEffect also cancels the effect when the dependencies change. But maybe that part could be dropped.

I'm just investigating the possibility of using onShow function. onModalShow is a method from a library that react-native-picker-select doesn't use. It looks promising and I believe that it's a very good suggestion.

[cubuspl42]

@0xmiroslav I replaced the time-based approach with onShow, like you suggested! I changed the name/contract of the flag stored in PickerStateContext, but that's semantics. I think that the whole solution actually simplified.

[0xmiros]

can you share 2 videos to clarify difference?

[cubuspl42]

It's just a small padding, which is "accidentally" the same as we use in Expensify. It just looks a bit better.

After the scroll happens, this is how it looks now:

How it looked before:

[cubuspl42]

Relevant line in Expensify: https://github.com/Expensify/App/blob/main/src/components/Form.js#L357

[0xmiros]

Looks good! Checklist will be done in app PR.
For now, please point latest commit hash in app PR.
@Julesssss will be back from OOO tomorrow and after this PR merged, we can simply update commit hash to merged one.

[Julesssss]

Looking good, just curious if the hardcoded height is suitable for iPad devices.

[0xmiros]

Looking good, just curious if the hardcoded height is suitable for iPad devices.

@Julesssss Portrait mode exactly matches hardcoded height but landscape mode height is smaller.
That hardcoded height (262) was introduced in #3
There was already discussion about that but didn't care about iPad landscape mode - https://github.com/Expensify/react-native-picker-select/pull/3/files#r1046091730 cc: @tgolen @chrispader

So 3 options here to go ahead

• find solution to get modal height programatically
• if it's iPad landscape, return xxx (measured value), if iPad portrait or iPhone, return 262
• leave as is now given that iPad is not officially supported platform

I tested on iPad mini 6th generation

NOTE: Currently team doesn't really care about iPad responsive.
i.e. Login page style broken but such issue was closed given it's not officially supported platform.

Screen.Recording.2023-04-25.at.12.22.10.PM.mov

[cubuspl42]

@Julesssss @0xmiroslav It's a good catch! Still, I think that this PR doesn't really introduce any regression in the discussed matter. The constant used for the modal was already present in the source code. Taking into consideration that @0xmiroslav is saying that even the sign in page doesn't support iPad properly, and iPad doesn't get first-class support, I think we don't have to improve the modal measurements, as the effect on iPad is not that bad.

[Julesssss]

Yeah this looks fine, so I agree we shouldn't care about this for now 👍

[s77rt]

If we show the Modal, onShow callback will be called and set modal as shown (isModalShown=true). Now if we unmount that modal (while it's shown) the state of the modal being shown won't be reset accordingly - state is stuck at true. We should have cleared the state on componentWillUnmount. (Coming from Expensify/App#23044)