-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 for #998 #1001
fix for #998 #1001
Conversation
Scanned through super quickly (on my way to lunch). At a glance the fix looks reasonable! And I love the fact that there's a test. The ternary within a boolean conditional looked strange but that's a nit. If you'd be willing to do the fix for column too, please do! |
i'm not a fan of the ternary + boolean too, an alternative is something like: this._recomputeScrollLeftFlag = scrollToColumn >= 0
? (this.state.scrollDirectionHorizontal === SCROLL_DIRECTION_FORWARD
? columnIndex <= scrollToColumn
: columnIndex >= scrollToColumn)
: false; Let me know if you prefer the above snippet and I'll update the pull request! |
Any chance this gets merged soonish ? |
@bvaughn sorry to bug you, but I'd really like to get this one fixed, when do you think it can be merged ? |
WOW, this is so much better for my chat use case, thanks @dcolens. BTW when I published this to our private NPM to try it out I had to run |
weekend is coming ! time for a merge ? |
Thanks for your contribution @dcolens, gonna carry out some testing on this an hopefully we can merge it very soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice! Thanks
@dcolens Do you mind putting the fix + tests and columns in this PR? That'll be amazing |
Wait nevermind. Merging |
Looks like this broke CI? |
This is a fix + tests for issue #998. Note that this is a partial fix as it covers only rows, submitting it for review. It it looks good, I can try doing the same for columns :-)