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

Child element does not fill parent height when using height: 100% #138

Open
Saikedo opened this issue Jan 9, 2022 · 8 comments
Open

Child element does not fill parent height when using height: 100% #138

Saikedo opened this issue Jan 9, 2022 · 8 comments

Comments

@Saikedo
Copy link
Contributor

Saikedo commented Jan 9, 2022

It seems like the height attribute just gets ignored for the child of the toolTip if you use percentage height.

Let`s take a look at this example. Here the parent view has a height of 300 and I am setting the child component of the tooltip,
The view and the text inside, to havea height of 100%. What I expect here to happen is that the view that wraps the text will span the entire height of the parent view but it seems like that '100%' gets ignored and the height of the inner view is determined by the height of the text inside.

<View style={{ width: 250, height: 300, backgroundColor: '#ff0000' }}>
  <Tooltip
    isVisible={showTip}
    content={
      <View>
        <Text> I am a tooltip </Text>
      </View>
    }
    onClose={() => setTip(false)}
    placement="top"
  >
    <View style={{ backgroundColor: 'yellow', width: '100%', height: '100%', flex: 1,}}>
      <Text>Child component</Text>
    </View>
  </Tooltip>
</View>

I tried everything that came to mind, playing with flex, flexGrow, and a bunch of other things but nothing worked. Basically, as soon as I use a numeric value for height it`s all good but this does not work with percentages?

Any ideas on how I can address this?

Thanks.

@Saikedo
Copy link
Contributor Author

Saikedo commented Jan 9, 2022

Ah, I see the reason now. This is how react-native-walkthrough-tooltip renders the children element in the parent

{hasChildren ? (
    <View ref={this.childWrapper} onLayout={this.measureChildRect}>
      {children}
    </View>
  ) : null}

Since this wraps children element in a view that does not stretch to fill the parent height of the tooltip, the children height of '100%' will only fill the 100% of this new view height.

It`s probably a good idea to add a way to control the style of this view externally right ?

@gregfenton
Copy link

Quite possibly. Do you want to do a PR for it?

@Saikedo
Copy link
Contributor Author

Saikedo commented Jan 9, 2022

@gregfenton Sure, ill make a PR for that.

Just to make sure, we are just adding a way to pass styles to the original children wrapper correct? Since there is already a prop called childrenWrapperStyle, I think calling it originalChildrenWrapperStyle should be clear enough.

Also, are there any potential issues that I should be aware of? Just making sure adding styles to that view won`t mess up the layout calculations in any way.

@gregfenton
Copy link

I think I'd consider calling it parentWrapperStyle given where it is going, and the fact that currently it is an empty value.

The above would have style={[this.props.parentWrapperStyle]} ? If not provided, should not affect the styles at all?

@Saikedo
Copy link
Contributor Author

Saikedo commented Jan 9, 2022

@gregfenton That's correct, empty by default so if parentWrapperStyle is undefined, then everything will stay the same.

@Saikedo
Copy link
Contributor Author

Saikedo commented Jan 9, 2022

@gregfenton Just opened a PR.

Did some quick tests, did not notice any alterations to the original behavior if nothing is passed.
And of course, now we are able to style the wrapper.

Quick question, should I also make a PR for #134 while I have the project open? Do you agree with the proposed solution there?

@Saikedo
Copy link
Contributor Author

Saikedo commented Mar 9, 2022

Hi, I have a Pull Request with these fixes and was wondering if there is any ETA for merging this in. I am stuck using a local version of the library because we really needed this change for our project wanted to see if I can help with anything to hasten the merge.

Thank you.

@Csierram96
Copy link

should this be closed now?

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

3 participants