-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Refactoring move.js of Designer #15378
Conversation
Signed-off-by: Mohit Kuri <mohit.kuri@research.iiit.ac.in>
For testing, I have tested this whole thing in general. Here is an example of the same: Using - document.getElementById('selector') Using - $('#selector')[0] Since this worked, I replicated the same here. @devenbansod Please review and provide feedback for the same. Will proceed further once the work till here is approved and we are good to go ahead. |
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.
@Bournvita1998 I've left a comment. It applies to all the changes.
js/designer/move.js
Outdated
@@ -101,7 +101,7 @@ DesignerMove.mouseDown = function (e) { | |||
} | |||
|
|||
if (curClick !== null) { | |||
document.getElementById('canvas').style.display = 'none'; | |||
$('#canvas')[0].style.display = 'none'; |
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.
@Bournvita1998 this would work for the elements selected with IDs only. I'd ideally use .css()
method to set the styles instead of manually selecting the first element and setting the native properties. IMO (even though it works) this current approach sort of doesn't use JQuery's power at all.
Using .css()
would auto-loop over all the selected elements, so would be easier to read when we're selecting based on input types, classes etc.
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 agree with @devenbansod . Don't try to hard-code things. As you're trying to refactor the code, try to do it in such a way that would let other developers work on the code without much changes to your code. It gets extremely difficult to manage such things on such a large scale.
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.
Okay, I will make these changes asap. Thanks @devenbansod, @shucon
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
Codecov Report
@@ Coverage Diff @@
## master #15378 +/- ##
============================================
- Coverage 52.99% 52.72% -0.27%
- Complexity 14047 14071 +24
============================================
Files 481 484 +3
Lines 62011 62131 +120
============================================
- Hits 32862 32760 -102
- Misses 29149 29371 +222 |
@devenbansod @shucon Please have a look now |
Signed-off-by: Mohit Kuri <mohit.kuri@research.iiit.ac.in>
I am done refactoring move.js once. Please share your reviews so that I can make the possible changes |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Signed-off-by: Mohit Kuri mohit.kuri@research.iiit.ac.in
Description
Refactored move.js of the designer.
Fixes #
Before submitting pull request, please review the following checklist:
Signed-off-by
line as described in our DCO. This ensures that the work you're submitting is your own creation.