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

#1164: Popover does not work on React16 when attached to body (closes #1164) #1165

Merged
merged 4 commits into from
Jan 3, 2018

Conversation

FrancescoCioria
Copy link
Contributor

@FrancescoCioria FrancescoCioria commented Dec 29, 2017

Closes #1164

Test Plan

tests performed

example with attachToBody works both on React16 and React15

cross browser compatibility

Should be compatible with every browser below. Check the ones you tested!

  • Google Chrome
  • Internet Explorer

tests not performed (domain coverage)

At times not everything can be tested, and writing what hasn't been tested is just as important as writing what has been tested.

An example of partial test is a field displaying 4 possible values. If 3 values are tested, with screenshots, and 1 is not, then it should be mentioned here.

@nemobot nemobot added bug bugs, defects, ... WIP Issues in progress labels Dec 29, 2017
@nemobot nemobot added in review Issues with an open PR waiting to be reviewed and removed WIP Issues in progress labels Dec 29, 2017
Copy link
Member

@giogonzo giogonzo left a comment

Choose a reason for hiding this comment

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

Looks 👍 , left a comment inline


// add pointer to popover node
this.popoverNode = this.containerNode.children[0];
// render Popover and save popover size in "onMount" cb (visible popover will be rendered in componentDidUpdate)
Copy link
Member

Choose a reason for hiding this comment

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

did you read this facebook/react#10309 (comment) ?
Seems slightly simpler (but same thing at an higher level)

@nemobot nemobot added waiting for merge Issues with an open PR ready to be merged and removed in review Issues with an open PR waiting to be reviewed labels Jan 3, 2018
@FrancescoCioria FrancescoCioria merged commit 6bee39c into master Jan 3, 2018
@nemobot
Copy link
Contributor

nemobot commented Jan 3, 2018

@nemobot nemobot removed the waiting for merge Issues with an open PR ready to be merged label Jan 3, 2018
@FrancescoCioria FrancescoCioria deleted the 1164-popover_does_not_work branch January 3, 2018 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs, defects, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants