-
Notifications
You must be signed in to change notification settings - Fork 15
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
Introduction to moveable api (resolves #214) #215
base: master
Are you sure you want to change the base?
Conversation
@@ -22,6 +22,16 @@ describe('Moveable component', () => { | |||
viewInstance.remove(); | |||
} | |||
|
|||
function mouseMoveTo(x, y) { |
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.
I would suggest to change this name to something like dragAndDropElement
and would pass element there explicitly. It will be more generic then and name will describe, what this function actually mock.
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.
Since drag and drop has the same events name wouldn't it be confusing ?
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.
So maybe somthing like dragAndDropMock
or something like that, because it in fact mock dragging and dropping, not only moving mouse.
Look pretty good, although I have few questions, suggestions. |
Codecov Report
@@ Coverage Diff @@
## master #215 +/- ##
==========================================
+ Coverage 20.89% 21.65% +0.76%
==========================================
Files 21 20 -1
Lines 512 434 -78
==========================================
- Hits 107 94 -13
+ Misses 405 340 -65
Continue to review full report at Codecov.
|
@@ -1,4 +1,5 @@ | |||
import { Model } from '../../core'; | |||
import { Model as BModel } from 'backbone'; |
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.
I would prefer to use more descriptive alias here - maybe BackboneModel
?
I have some minnor question, but after getting some answers I'm going to merge it - we can make it better in future, but it takes us really lot of time and we need to speed up with development :). |
616a0dd
to
0e9bf2d
Compare
No description provided.