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

Scroll to selected diff line only if head of selection changes #1281

Merged
merged 1 commit into from Jan 13, 2018

Conversation

Projects
None yet
2 participants
@kuychaco
Member

kuychaco commented Jan 12, 2018

Previously, componentDidUpdate was being called when anonymous
function props were updated, triggering scrolling at undesired times.
Instead, now we specifically check to see if the relevant props have
changed before scrolling.

Fixes #1226
Fixes #884

Scroll to selected diff line only if head of selection changes
Previously, `componentDidUpdate` was being called when anonymous
function props were updated, triggering scrolling at undesired times.
Instead, now we specifically check to see if the relevant props have
changed before scrolling.

Admittedly, I'm not sure what the purpose of the `selectedLine` branch
of the original conditional was for. I traced its addition back to the
"open diff from editor" feature I added 8 months ago, but manual testing
of that feature seems to work without it. So I'm removing it and hoping
that no regression is introduced, and taking a mental note to write more
informative commit messages in the future.

@kuychaco kuychaco requested a review from BinaryMuse Jan 12, 2018

@BinaryMuse

👍 Looks good other than one question: we dropped the setTimeout — I assume it's not necessary anymore?

@kuychaco

This comment has been minimized.

Member

kuychaco commented Jan 13, 2018

we dropped the setTimeout — I assume it's not necessary anymore?

Yeah we've dropped that branch of the conditional entirely. Tbh it's still unclear why we needed it in the first place. And the conditional branch that uses it seems to be for the "Open diff from editor" feature, but removing it doesn't seem to break anything because the hunkLine branch handles the case. So I just dropped that logic entirely as it seems redundant.

@kuychaco kuychaco merged commit 32b9199 into master Jan 13, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kuychaco kuychaco deleted the ku-fix-scroll-bugs branch Jan 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment