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 init.js of Designer #15338
Conversation
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
Good job. |
@@ -54,22 +54,22 @@ AJAX.registerOnload('designer/init.js', function () { | |||
} | |||
|
|||
$('#query_Aggregate_Button').on('click', function () { | |||
document.getElementById('query_Aggregate').style.display = 'none'; | |||
$('#query_Aggregate').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 shouldn't we be using $("#id").css("display", "none");
or $("#id").hide();
here?
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.
@devenbansod I didn't find a good article comparing the difference between these 3. I know they all does the same job but no idea what's the difference. Can you help me knowing the difference between them?
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.
@devenbansod, for now, I am thinking of moving ahead making the changes in a similar way as I have done here. If required, we will change them again.
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 have you tested this in the browser?
AFAIK, $('selector')
returns an array-like JQuery-native object and doesn't return a native DOM element. So if I'm not mistaken, this code change would throw an error: Uncaught TypeError: Cannot set property 'display' of undefined
in the console.
Ref for the css
API: https://api.jquery.com/css/
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.
@devenbansod Yeah you are right here. $('selector') will return an array of all possible matches but document.getElementById('ID') returns just the first match it finds. To resolve this error we can either do the things as you suggested in your first comment or we can change $('selector') to $('selector')[0]. I found this on stackoverflow explaining the difference between jQuery object and DOM elements.
Also, I found the same issue in my previously created PRs too, which need to be changed soon. I am thinking of replacing them using the css format like: $("#id").css("display", "none"); is that fine to move 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.
Sure, please 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.
This was indeed invalid: #16960
Gosh, why did I approve this invalid code.. 🤦🏻 |
Signed-off-by: Bournvita1998 mohit.kuri@research.iiit.ac.in
Description
Refactoring in the init.js of the designer section
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.