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

Make rowIndex and columnIndex optional #174

Merged
merged 2 commits into from
Mar 16, 2019

Conversation

jgoz
Copy link
Contributor

@jgoz jgoz commented Mar 12, 2019

Closes #173.

Update scrollToItem to make rowIndex and columnIndex optional. If unspecified (or invalid, e.g., negative value), the current value for scrollTop or scrollLeft will be used.

  • Updated tests
  • Updated documentation

horizontalScrollbarSize
),
scrollLeft:
columnIndex !== undefined && columnIndex >= 0
Copy link
Owner

Choose a reason for hiding this comment

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

Why is the >= 0 needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's not needed for this feature, but negative indexes will not produce valid results, especially with VariableSizeGrid. I can remove the check if you prefer to leave the behaviour undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bvaughn Specifically, for VariableSizeGrid, itemMetadata will be undefined, which will throw an error when calculating maxOffset.

@jgoz jgoz force-pushed the 173-optional-scrollItem-index branch from d4a9dec to 665957e Compare March 13, 2019 03:46
@jgoz
Copy link
Contributor Author

jgoz commented Mar 13, 2019

@bvaughn Removed the >= 0 as it wasn't directly relevant to this enhancement.

// Scroll down to row 10, without changing scrollLeft
rendered.getInstance().scrollToItem({ rowIndex: 10, align: 'auto' });
// Scroll left to column 0, without changing scrollTop
rendered.getInstance().scrollToItem({ columnIndex: 0, align: 'auto' });
Copy link
Owner

Choose a reason for hiding this comment

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

You aren't testing anything between these scrollToItem calls. That seems not great.

Copy link
Owner

Choose a reason for hiding this comment

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

Whoops. I misremembered how my own tests were written 😆 you're expecting on the mock calls below. 👍

@bvaughn bvaughn merged commit 665957e into bvaughn:master Mar 16, 2019
Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This looks nice. Thanks

@bvaughn
Copy link
Owner

bvaughn commented Mar 16, 2019

Just released with v1.7.0

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.

None yet

2 participants