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.

Sign up for GitHub

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

Jump to bottom

rm deprecated componentWillReceiveProps #208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rm deprecated componentWillReceiveProps #208

wants to merge 1 commit into from

Conversation

a-x-
Copy link

@a-x- a-x- commented Aug 15, 2019

fixes #207

sajera
sajera approved these changes Oct 7, 2019
View reviewed changes
Copy link

@sajera sajera left a comment

Choose a reason for hiding this comment

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

Looks alright

@slonofanya
Copy link

When it will be Merged? I really need it

@xxx
Copy link

xxx commented Dec 11, 2019 •
edited

after applying this patch, popovers no longer open for me. There are no errors or warnings in the console. The HEAD of master (905e754) works fine.

edit:

The ref's containerEl is always null with this change applied.

may be related to https://stackoverflow.com/a/50019873/340799

edit 2:

splitting the open/enter and close/exit calls resolved this for me. It can't be assumed that the state will toggle everytime:

  componentDidUpdate(propsPrev, statePrev) {
    //log(`Component did update!`)
    const willOpen = !propsPrev.isOpen && this.props.isOpen
    const willClose = propsPrev.isOpen && !this.props.isOpen

    if (willOpen) {
      this.open()
    } else if (willClose) {
      this.close()
    }

    const didOpen = !statePrev.toggle && this.state.toggle
    const didClose = statePrev.toggle && !this.state.toggle

    if (didOpen) {
      this.enter()
    }
    else if (didClose) {
      this.exit()
    }
  }

@Serginyo90
Copy link

Please merge

abrad45
abrad45 reviewed Jan 30, 2020
View reviewed changes
Copy link

@abrad45 abrad45 left a comment

Choose a reason for hiding this comment

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

lgtm

@xxx xxx mentioned this pull request Jan 30, 2020
Merged
@stclairdaniel
Copy link

@xxx Is this going to get a release?

@pinkynrg
Copy link

@xxx will this be merged soon?

@jmansor
Copy link

jmansor commented Oct 23, 2020

@xxx will this be merged soon?

+1

@edraghiciu-vks
Copy link

Any news about this? Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abrad45 abrad45 abrad45 left review comments

@slonofanya slonofanya slonofanya approved these changes

@sajera sajera sajera approved these changes

@gilgold gilgold gilgold approved these changes

@ichabanivan ichabanivan ichabanivan approved these changes

@zalcode zalcode zalcode approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warning: componentWillReceiveProps has been renamed