Skip to content
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.

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

Android devices positioning popover in center of screen #28

Closed
DaneEveritt opened this issue Nov 6, 2018 · 15 comments
Closed

Android devices positioning popover in center of screen #28

DaneEveritt opened this issue Nov 6, 2018 · 15 comments

Comments

@DaneEveritt
Copy link

So I've got a bit of a weird bug going on that I can't quite seem to figure out. On iOS devices, this popover appears correctly at the top of the view. But on android devices, it gets shoved into the middle, and the arrow is dead center on the screen:

img_0335

However, this popover which is attached to an absolutely positioned element on the screen shows up correctly on both devices?

img_0336

Sorry about the cropping, its not an open project. Happy to provide as much information as I can, just not sure exactly where to start in terms of providing helpful content.

@DaneEveritt
Copy link
Author

Heres the output from debug=true which seems to indicate its not properly getting the ref even though the other (the bottom picture above), is?

componentWillReceiveProps - Awaiting popover show
Popover.js:61 setDefaultDisplayArea - newDisplayArea: {"x":10,"y":10,"width":340,"height":596}
Popover.js:61 measureContent - Showing Popover - requestedContentSize: {"height":52.5,"width":317.5,"y":0,"x":0}
Popover.js:61 computeGeometry - displayArea: {"x":10,"y":10,"width":340,"height":596}
Popover.js:61 computeGeometry - fromRect: {"y":null}
Popover.js:61 measureContent - Showing Popover - geom: {"popoverOrigin":{"x":21.25,"y":281.75},"anchorPoint":{"x":180,"y":308},"forcedContentSize":{"width":340,"height":596},"viewLargerThanDisplayArea":{"width":false,"height":false}}
Popover.js:61 setDefaultDisplayArea (inside calculateRect callback) - fromRect: {"y":null}
Popover.js:61 setDefaultDisplayArea (inside calculateRect callback) - getDisplayArea(): {"x":10,"y":10,"width":340,"height":596}
Popover.js:61 setDefaultDisplayArea (inside calculateRect callback) - displayAreaStore: {"x":10,"y":10,"width":340,"height":596}```

@DaneEveritt
Copy link
Author

Alright, narrow down the weird behavior. Turns out if the component I was wrapped is a <View> at the highest level it causes this problem. Removing the view tag and returning a react fragment for example fixes it, but returning a view inside a react fragment still leaves it broken.

I suppose it would help to show how I'm generating these popovers. I'm using a component that wraps whatever element I want to put the popover on. This is the code that handles the wrapping and sets the ref on the child so that the popover can be assigned to it.

  render () {
    let child: React.Element<any> | void;
    if (this.props.children) {
      child = React.Children.only(this.props.children);
    }

    return (
      <React.Fragment>
        {child && React.cloneElement(child, {ref: (r) => this.elementRef = r})}
        <Popover
          isVisible={!this.state.waitingOnDelay && this.isVisible()}
          fromView={this.elementRef}
          onClose={this.onClose.bind(this)}
          doneClosingCallback={this.onCloseCallback.bind(this)}
          animationConfig={{
            duration: 200,
            easing: Easing.inOut(Easing.quad),
          }}
          popoverStyle={{
            backgroundColor: 'rgba(0, 0, 0, 0.6)',
            borderWidth: 1,
            borderColor: colors.black,
            borderRadius: 5,
          }}
        >
          <Text style={[textStyle.default, textStyle.callout, {padding: BASE_MARGIN, color: colors.white}]}>
            {this.props.description}
          </Text>
        </Popover>
      </React.Fragment>
    );
  }

Here is an example of where this bug was occurring, and how I managed to fix it in one instance. Theres a lot of missing code, I'm sorry, but I've included the relevant render functions to help explain hopefully.

<TutorialPopover
  name={TUTORIAL_TYPES.TYPE_JOB_OPTIONS}
  showAfter={TUTORIAL_TYPES.TYPE_BEGIN_JOB}
  description={'This button will bring up a list of options available for this job.\n\nSome options only show up when a job is in progress, or after a job is marked as complete.'}
>
  <GreyButton onPress={() => this._actionSheet.show()} icon={'list'}>
    Options
  </GreyButton>
</TutorialPopover>

GreyButton.js

render () {
  return <BaseButton color={'grey'} {...this.props}/>
}

BaseButton.js (this version causes Android to place the popover in the center of the screen)

  render () {
    return (
      <View>
        <Touchable
          onPress={this.props.onPress}
          disabled={this.props.loading || this.props.disabled}
          activeOpacity={0.6}
          background={Touchable.Ripple(colors[this.props.color].darker)}
          style={[style.button, buttonStyle[`${this.props.color}Button`], (this.props.loading || this.props.disabled) && {opacity: 0.3}]}
        >
         ...
        </Touchable>
      </View>
    );
  }

If I remove the <View> tag from the BaseButton component so that Touchable is the wrapper component, the popover shows in the correct spot.

@SteffeyDev
Copy link
Owner

Do you ensure that isVisible is not set to true until after this.elementRef is set? Android ref resolution may be slower than on iOS.

@SteffeyDev
Copy link
Owner

In other words, make sure that when you set isVisible to true, you first check to make sure this.elementRef is not undefined, otherwise delaying showing the popover until it is defined

@DaneEveritt
Copy link
Author

@SteffeyDev yeah, I am waiting for the element ref. isVisible() has a few steps, but the first is checking for this.state.canShowPopover which is set to true once the component has mounted and there is an element ref.

I also added some logging when that state change occurs, and there is an elementRef set at that point for them all, both the broken ones and working ones. I did more testing this morning and managed to get another working by removing some <View> tags around the component's render. Once those are out of the way it begins working fine.

I'm quite stumped.

@DaneEveritt
Copy link
Author

DaneEveritt commented Nov 7, 2018

So I completely skipped the whole special component I made to see if there was something weird going on there. I don't believe that is the case. Here is an example of this happening without anything special. Just a single component and a popover.

export default class JobInformationBar extends React.PureComponent<Props, State> {
  _stackedRef;
  state: State = { isMounted: false };
  componentDidMount () {
    if (this._stackedRef) {
      this.setState({isMounted: true});
    }
  }

  render () {
    return (
      <View style={{flex: 1}}>
        <StackedBlock
          ref={(ref) => this._stackedRef = ref}
          topText={this.props.time.toFixed(2).toString()}
          bottomText={'Hours'}
          style={{marginHorizontal: BASE_MARGIN}}
        />
        <Popover fromView={this._stackedRef} isVisible={this.state.isMounted}>
          <Text>Some text</Text>
        </Popover>
      </View>
    );
  }
}

StackedBlock.js

// @flow
import React from 'react';
import { Text, View } from 'react-native';
import { BASE_MARGIN, colors, textStyle } from './../styles';

interface Props {
  topText: string,
  bottomText: string,
}

export default class StackedBlock extends React.PureComponent<Props> {
  render () {
    const {bottomText, topText, ...props} = this.props;

    return (
      <View {...props}>
        <Text style={[textStyle.default, textStyle.title, {textAlign: 'center', color: colors.green.dark, paddingBottom: BASE_MARGIN / 4}]}>
          {topText.toUpperCase()}
        </Text>
        <Text style={[textStyle.default, textStyle.footnote, {color: colors.grey.darker, textTransform: 'uppercase'}]}>
          {bottomText.toUpperCase()}
        </Text>
      </View>
    );
  }
}

@SteffeyDev
Copy link
Owner

SteffeyDev commented Nov 7, 2018

Ok, try this in your componentDidMount:

NativeModules.UIManager.measure(findNodeHandle(this.elementRef), (x0, y0, width, height, x, y) => console.log(JSON.stringify({x, y, width, height})));

That’s exactly what my component is doing to get the position of the view, if any of those 4 values are undefined it would explain the behavior you are seeing. You’ll need the appropriate imports from react-native to run this code.

@DaneEveritt
Copy link
Author

DaneEveritt commented Nov 7, 2018

So on android I get {} back, on iOS I get back all of the properties (they're all 0, but it works fine).

Does this mean theres still some issue going on with the ref? I should mention I did this test on that last example I posted, outside my special component since it seems easier to test and less things going on around it.

@DaneEveritt
Copy link
Author

I fixed it! I found this issue on Github and added renderToHardwareTextureAndroid={true} to my <StackedBlock> and now it is working correctly.

@DaneEveritt
Copy link
Author

I've addressed this for the time being on my code by just adding collapsable={false} to each child automatically from my custom popover component when I'm doing the child clone. This seems to have fixed all of the remaining issues, and would explain why things that weren't wrapped in a view were working correctly.

@SteffeyDev
Copy link
Owner

Glad you were able to fix! Unfortunately, my component is restricted by what RN can give me for values. However, I think I can create some more useful warning and documentation to help people who run into these sorts of problems in the future.

@DaneEveritt
Copy link
Author

Yeah, don't think this is an issue with this library so much as it is an issue with RN. A note somewhere about adding collapsable to view elements you're trying to position against might be all thats needed.

Thanks again, library has saved me a ton of time.

SteffeyDev added a commit that referenced this issue Nov 7, 2018
This should help clarify a few things that we learned in those issues
@SteffeyDev
Copy link
Owner

Can you check the new documentation to see if it is correct? https://github.com/SteffeyDev/react-native-popover-view/blob/master/README.md#troubleshooting

@DaneEveritt
Copy link
Author

Looks good, I ended up not needing the renderToAndroid prop, just the collapsable. :)

@SteffeyDev
Copy link
Owner

Ok, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants