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

Fix case when scrollToPosition will not update internal state in React >= 16.4. #1288

Merged
merged 1 commit into from
May 3, 2019

Conversation

john-veresk
Copy link
Contributor

Fixes #1179 #1226

In cases when the Grid component is controlled (WindowScroller, MultiGrid) fields scrollLeft and scrollTop were not updated on setState call. getDerivedStateFromProps
is called before every render starting from React 16.4 and those fields were overridden by previous props.

it also fixes Lists scrollToRow behavior with the same problem.

Before submitting a pull request, please complete the following checklist:

  • The existing test suites (npm test) all pass
  • For any new features or bug fixes, both positive and negative test cases have been added
    This one can only be reproduced in React >= 16.4. What I should do?
  • For any new features, documentation has been added
  • For any documentation changes, the text has been proofread and is clear to both experienced users and beginners.
  • Format your code with prettier (npm run prettier).
  • Run the Flow typechecks (npm run typecheck).

…t >= 16.4.

In cases when the Grid component is controlled (WindowScroller, MultiGrid) fields scrollLeft and scrollTop ware not updated on setState call. getDerivedStateFromProps
is called before every render starting from React 16.4 and those fields were overridden by previous props.
@somi92
Copy link

somi92 commented Dec 15, 2018

Nice!

Did you maybe try to tackle #1170? Unfortunately, it still persists and I think the cause is somewhat similar. I took a bit of a radical approach which involves breaking changes (see #999 (comment) for more details). It would be nice if #1170 could be fixed within the confines of the current API, but I haven't found the way.

@john-veresk
Copy link
Contributor Author

@somi92 Thank you for your answer, your PR inspired me to make my own, you did a great work, but it was clear signal from maintainers that it has low chance to get merged. I've decided to try fix it other way and hope it will be merged in a near future.

I'll take a look at #1170.

@wuweiweiwu wuweiweiwu self-assigned this Dec 26, 2018
@uncleramsay
Copy link

Is there any update on this? I'm keen to get a solution to #1179

@john-veresk
Copy link
Contributor Author

@uncleramsay I'm also waiting for this PR to merge. It seems the project is not maintained.

@uncleramsay
Copy link

@wuweiweiwu @bvaughn any thoughts here?

Copy link
Contributor

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

Can you run prettier and add tests?

@john-veresk
Copy link
Contributor Author

john-veresk commented Feb 24, 2019

Can you run prettier and add tests?

"peerDependencies": { "react": "^15.3.0 || ^16.0.0-alpha", "react-dom": "^15.3.0 || ^16.0.0-alpha" }

@wuweiweiwu This one can only be reproduced in React >= 16.4. What I should do?

Update: I've run prettier and it haven't introduced any changes. npm run lint gives only one old warning.

@uncleramsay
Copy link

@wuweiweiwu just pinging you on this one! I really need this fix in for my application :)

@uncleramsay
Copy link

@wuweiweiwu ping!

@wuweiweiwu
Copy link
Contributor

The only thing is that im worried that #1320 will have merge conflicts. I have some free time next week so ill see what i can do

@john-veresk
Copy link
Contributor Author

I think this PR is more for 9.22.0 release while #1320 is for 10.0.0 due to breaking changes.

In either case, I would be glad if #1320 will be released because it is also fixes this issue (with a little work on user side).

@uncleramsay
Copy link

@wuweiweiwu any update?

@MartinAyvazyan
Copy link

@wuweiweiwu @bvaughn any updates?

@wuweiweiwu
Copy link
Contributor

I'll test and release later this week! Will update thread when that happens! Thanks for your patience

@MartinAyvazyan
Copy link

Thanks for response dear @wuweiweiwu . We have broken functionality because of this issue. So I'm impatiently waiting for your release.

@MartinAyvazyan
Copy link

@wuweiweiwu I think you have forgotten about this.
P.S. sorry for spamming 😄

@ghazaryanhayk
Copy link

Hey @wuweiweiwu
Is there any news for this PR??
If it's possible, can you please let us know when it will be merged?

@uncleramsay
Copy link

@wuweiweiwu can we get this released please?

@uncleramsay
Copy link

@wuweiweiwu pretty please? :)

@uncleramsay
Copy link

@wuweiweiwu any update? Lots of people here waiting for the fix. Please let me know if I can help in any way...

@wuweiweiwu wuweiweiwu merged commit 32855b8 into bvaughn:master May 3, 2019
@wuweiweiwu
Copy link
Contributor

@uncleramsay Done!!

Will release in a couple of days :) Thanks for your patience

@MartinAyvazyan
Copy link

Can't believe thanks a lot ❤️

@wuweiweiwu
Copy link
Contributor

Released in 9.21.1!!!!!

wuweiweiwu added a commit that referenced this pull request Nov 9, 2019
wuweiweiwu added a commit that referenced this pull request Nov 10, 2019
@michchan
Copy link

michchan commented Mar 2, 2020

NOT Working with WindowScroller

Hi @Jaycrypto
Have you fixed it for using with WindowScroller?
For my case, I tested it works when NOT using the window scroller (for both scrollToRow and scrollToPosition),
while it doesn't work when using with WindowScroller.

"react": "^16.8.5",
"react-dom": "^16.8.5",
"react-virtualized": "^9.21.2"

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.

Calling scrollToRow() on List no longer works after updating React to v16.4.1
7 participants