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

Replay recent PR improving animatedHeight prop #585

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pandaiolo
Copy link
Contributor

I believe there is no link between this code and the recent issues. And it's a valuable improvement.

Replays the following PRs from the old master:

tuomohopia and others added 4 commits February 3, 2020 13:55
This was an issue we had as well, this PR was the fix for us during the project, I have accepted it to be the official fix for this issue.
@oliviertassinari
Copy link
Owner

oliviertassinari commented Feb 3, 2020

I suspect this one introduces a regression. I don't have time to look into it. Unless somebody is willing to significantly dedicate time to this component (more than +20h/month), I would be in favor of only handling changes regarding code infrastructure and to freeze any change that might impact the behavior of the component. The logic has been running fine for years now, check react-select, it's full of open issues and buggy edge cases and yet, it doesn't prevent developers from using it. We can go with the same approach here.

Changes that seem fine to push further:

@pandaiolo
Copy link
Contributor Author

Do you mind sharing your concern about the suspecte regression? It felt that this code was modifying only the animateHeight scope of this component. Anyway I guess more tests are needed... unfortunately I don't have more time to dedicate either :)

@oliviertassinari
Copy link
Owner

I have noticed that the height animation demo was no longer running the animation on the master-off-track branch. I could be coming from these changes, modifying the default behavior.

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

Successfully merging this pull request may close these issues.

3 participants