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

fix for #998 Reseting scrollUp/scrollLeft to scrollToRow/scrollToColumn incorrectly #1154

Merged
merged 1 commit into from
Jun 27, 2018
Merged

fix for #998 Reseting scrollUp/scrollLeft to scrollToRow/scrollToColumn incorrectly #1154

merged 1 commit into from
Jun 27, 2018

Conversation

mengdage
Copy link
Contributor

@mengdage mengdage commented Jun 25, 2018 •
edited by wuweiweiwu
Loading

The original fix #1001 for the issue #998 is reverted because of failing tests due to upgrading to React 16. I create two tests to verify that scrollUp/scrollLeft are not reset to scrollToRow/scrollToColumn when recompute grid size with index less than scrollToIndex.

closes #998

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
  • 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).

Here is a short checklist of additional things to keep in mind before submitting:

  • Please make sure your pull request description makes it very clear what you're trying to accomplish. If it's a bug fix, please also provide a failing test case (if possible). In either case, please add additional unit test coverage for your changes. :)
  • Be sure you have notifications setup so that you'll see my code review responses. (I may ask you to make some adjustments before merging.)

@mengdage mengdage changed the title fix for #998 Reseting scrollUp to scrollToIndex fix for #998 Reseting scrollUp to scrollToIndex incorrectly Jun 25, 2018
@mengdage mengdage mentioned this pull request Jun 25, 2018
Closed
@mengdage
Copy link
Contributor Author

mengdage commented Jun 25, 2018 •
edited
Loading

My problem is exactly the same as #1148. This PR should solve it.

@mengdage mengdage changed the title fix for #998 Reseting scrollUp to scrollToIndex incorrectly fix for #998 Reseting scrollUp/scrollLeft to scrollToRow/scrollToColumn incorrectly Jun 25, 2018
@aem
Copy link
Contributor

aem commented Jun 25, 2018

#998 specifically discusses scrollToIndex, can we get a test case verifying that specific behavior being fixed?

@mengdage
Copy link
Contributor Author

mengdage commented Jun 25, 2018 •
edited
Loading

@aem That's what the tests in this PR do. If you revert the changes, these two tests should fail.

@aem
Copy link
Contributor

aem commented Jun 25, 2018

sorry. didn't scroll down far enough. that's my fault

@Tin-Nguyen
Copy link

@aem , we got the same issue and we need this PR is merged soon to resolve our issue. I think the change is good enough. If you think we have another way to fix, let us know when possibly we can fix the issue. Thanks

wuweiweiwu
wuweiweiwu approved these changes Jun 27, 2018
View reviewed changes
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.

Awesome! Nice test case. I ran it locally and everything looks good! Thanks for contributing.

@wuweiweiwu
Copy link
Contributor

wuweiweiwu commented Jun 27, 2018 •
edited
Loading

cc: @dcolens. I didnt get a chance to fix the broken tests after the rewrite. But this MR will have it in! Nevertheless, we appreciate your contributions

@wuweiweiwu wuweiweiwu merged commit 8738097 into bvaughn:master Jun 27, 2018
@sytranvn
Copy link

Hi there, can you brief when will you patch this?

@wuweiweiwu
Copy link
Contributor

@sytranvn I would say within a week!

@sytranvn
Copy link

Thanks @wuweiweiwu

@sytranvn
Copy link

sytranvn commented Jul 9, 2018

Hey @wuweiweiwu,
I haven't seen this patch released. Can you tell me when will this patch be released?
Thank you :)

@wuweiweiwu
Copy link
Contributor

@sytranvn there are some more bugs that I want to fix before the next patch release. Thanks for your patience

@awkaiser
Copy link

awkaiser commented Jul 9, 2018

@wuweiweiwu I'd also love to see a patch release for this. Unless you're addressing new bugs that have only been present on master since 9.20.0, it'd be great if you could release now, and release again with those additional bug fixes later.

🍻

@wuweiweiwu
Copy link
Contributor

I see. That makes sense. I'll try to release tomorrow!

@wuweiweiwu wuweiweiwu mentioned this pull request Jul 11, 2018
Merged
@wuweiweiwu
Copy link
Contributor

Released in version 9.20.1!

@dependencies dependencies bot mentioned this pull request Jul 11, 2018
Merged
errendir added a commit to IdeaFlowCo/react-virtualized that referenced this pull request Oct 24, 2018
@errendir
Merge remote-tracking branch 'bvaughn/master' into invalidate-caches
b16132f 
* bvaughn/master: (54 commits)
  Update version and changelog for 9.21.0 release (bvaughn#1252)
  chore: update lockfile
  Update ci badge (bvaughn#1227)
  Allow users to override default table row styles (bvaughn#1175)
  Add onColumnClick to Table (bvaughn#1207)
  remove unused variable in Masonry.example.js (bvaughn#1218)
  Fix Table aria attributes (bvaughn#1208)
  Fix typo in CellMeasurer.DynamicHeightTableColumn.example.js (bvaughn#1190)
  Update usingAutoSizer.md (bvaughn#1186)
  Add an extra check for an e.target.className.indexOf function (bvaughn#1210)
  Fix broken Slack badge image (bvaughn#1205)
  docs(CellMeasurer): fix `import` statement (bvaughn#1187)
  Added new friend (bvaughn#1197)
  Fix createMultiSort bug (bvaughn#1051)
  adding new usecase example and fix some typos (bvaughn#1168)
  Updating version to 9.20.1
  Update changelog for the 9.20.1 release (bvaughn#1167)
  Prevent early debounceScrollEndedCallback when there is a slow render (bvaughn#1141)
  removing sideEffects (bvaughn#1163)
  fix for bvaughn#998 with test cases (bvaughn#1154)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wuweiweiwu wuweiweiwu wuweiweiwu approved these changes

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

Successfully merging this pull request may close these issues.

scroll up an infiniteLoader that has scrollToIndex set bounces back to scrollToIndex
6 participants
@mengdage @aem @Tin-Nguyen @wuweiweiwu @sytranvn @awkaiser