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

fix: check onMouseDown event is cancelable before calling event.preventDefault() #133

Merged
merged 6 commits into from
Dec 13, 2022

Conversation

danhobbs75
Copy link
Contributor

The same test as for the resizable directive to stop console warnings being shown when calling preventDefault()

@danhobbs75
Copy link
Contributor Author

I can see it's still got 5 commits from the previous PR. I'm not sure how that's happened. I did update my forked repo from this one before making the commit with the latest one-line change. I'm not sure how to delete them, or if that's even the right thing to do. Please feel free to reject the PR if I need to do something else on my fork repo first.

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

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

Thank you!!

@mattlewis92
Copy link
Owner

I can see it's still got 5 commits from the previous PR. I'm not sure how that's happened. I did update my forked repo from this one before making the commit with the latest one-line change. I'm not sure how to delete them, or if that's even the right thing to do. Please feel free to reject the PR if I need to do something else on my fork repo first.

You're good! I'll be squash merging this so as long as the "Files changed" tab is correct we don't need to worry about the git history 😄

@mattlewis92 mattlewis92 merged commit 4438bab into mattlewis92:main Dec 13, 2022
@mattlewis92
Copy link
Owner

Released as 7.0.2!

@danhobbs75
Copy link
Contributor Author

Released as 7.0.2!

NOW it's working smoothly. Thanks for dealing with the merge admin again. :-)

@mattlewis92
Copy link
Owner

Released as 7.0.2!

NOW it's working smoothly. Thanks for dealing with the merge admin again. :-)

This makes more sense to me now, as the try catch should have been silencing this error. But I guess we missed this place 😄

@danhobbs75
Copy link
Contributor Author

Released as 7.0.2!

NOW it's working smoothly. Thanks for dealing with the merge admin again. :-)

This makes more sense to me now, as the try catch should have been silencing this error. But I guess we missed this place 😄

It looks like the behaviour is that try/catch will stop an error going to the console, but warnings are still reported.

And I do recall that I noticed this second instance when I first looked at the code, but in between getting to grips with everything else (plus it was already late on Sunday night) I forgot about it. Oh well. I did also learn that spurious error messages from git were really caused by me not following commit naming standards (the 'fix:' prefix was missing initially) and I wasted a LOT of time reinstalling git because the error message made it appear that cygwin was missing. A classic developer tale of woe solved by actually reading the output from git which gave the game away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants