Skip to content

Conversation

@acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 7, 2017

This fixes a snapshot test regression introduced by #10008. I had noticed this change and believed the new behavior was correct, but upon further investigation I was wrong. This reverts the snapshot test and fixes it accordingly.

This fixes a snapshot test regression introduced by facebook#10008. I had
noticed this change and believe the new behavior was correct, but upon
further investigation I was wrong. This reverts the snapshot test and
fixes it accordingly.
Copy link
Contributor

@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.

@acdlite and I chatted about this out of band. I'm not super familiar with the surrounding code but this change seems okay. 👍

@acdlite acdlite merged commit ce7c014 into facebook:master Jul 7, 2017
@acdlite acdlite deleted the fixsnapshotregression branch July 7, 2017 22:34
@sophiebits
Copy link
Collaborator

Does it make sense to add an explicit test for this?

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 7, 2017

@spicyj Yes, though I can't think of a good one that isn't coupled to implementation. Ideally we'd cover this with a generative test suite, like with the triangle tester.

@sophiebits
Copy link
Collaborator

Is this the selection.extend bug that surfaced in the sync? I thought it was but now maybe it's not. :) If it is then we could reduce that maybe? (If not, is there a fix for that somewhere?)

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 7, 2017

It's a separate bug that happened to manifest as triggering the selection.extend bug in www.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants