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

Clean up selecting guard logic #207

Closed
wants to merge 1 commit into from

Conversation

marklawlor
Copy link
Contributor

Proposed Change:

Clean up the selecting logic to be handled by interactjs.

Inspired by #206 (comment).

Change type

  • Bugfix
  • New feature
  • Refactor

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link
Collaborator

@lilfolr lilfolr left a comment

Choose a reason for hiding this comment

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

Tried this locally and it seems to break the selecting.
Can you confirm if it works for you? Haven't had time to check why it isnt working

@marklawlor
Copy link
Contributor Author

Tried this locally and it seems to break the selecting.
Can you confirm if it works for you? Haven't had time to check why it isnt working

Selecting is working for us, but that maybe because we have also changed the event listeners on the row (eg #206 & other changes not pushed yet). Looking at our patches, we have removed the row events completely - which might be why it works for us. I'm guessing the e.preventDefault() in _handleItemRowEvent should be after the if (this.selecting) {} guard, as the onMouseOver and onMouseLeave events will be cancelled and never bubble to interactjs

We're in the crunch towards a deadline, but I'll come back and have a look at this afterwards. These PRs are me just trying to push some of our changes upstream.

@lilfolr
Copy link
Collaborator

lilfolr commented Jan 24, 2021

Awesome thanks - really appreciate you taking the time to do so

@marklawlor marklawlor closed this Jul 25, 2021
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.

2 participants