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

Improving designer #15165

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
4 participants
@Bournvita1998
Copy link
Contributor

commented Apr 8, 2019

Description

#15163
Signed-off-by: Bournvita1998 mohit.kuri@research.iiit.ac.in

Refactored 2 JS files of Designer

Fixes #

Before submitting pull request, please review the following checklist:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.
@phpmyadmin-bot

This comment has been minimized.

Copy link

commented on 3fd6103 Apr 8, 2019

This commit is missing Signed-Off-By line to indicate that you agree with phpMyAdmin Developer's Certificate of Origin. Please check contributing documentation for more information.

refactored 2 files
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
@williamdes

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@Bournvita1998 I see that you have made great improvements, can you fix https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/517394475#L645 before I review the PR please ?

@@ -2,8 +2,7 @@ var designer_tables = [{ name: 'pdf_pages', key: 'pg_nr', auto_inc: true },
{ name: 'table_coords', key: 'id', auto_inc: true }];

var DesignerOfflineDB = (function () {
var designerDB = {};
var datastore = null;
var designerDB = {}, datastore = null;

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Apr 8, 2019

Member

You should split var declarations into multiple statements.

This comment has been minimized.

Copy link
@Bournvita1998

Bournvita1998 Apr 8, 2019

Author Contributor

I remembered that we should not declare the variables in multiple lines as per good coding practices. But found that its wrong :(. Thus reverted the changes. Also due to the same reason its showing linting error (which I have removed rn, will be displayed in the new commit).

document.getElementById('query_where').style.zIndex = '103';
document.getElementById('query_where').style.visibility = 'visible';
document.getElementById('query_where').style.display = 'block';
$("#eQuery").value = history_array[index].get_obj().getquery(); // get_obj(),get_query might be removed

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Apr 8, 2019

Member

Why replace document.getElementById with the jQuery ID selector?

This comment has been minimized.

Copy link
@Bournvita1998

Bournvita1998 Apr 8, 2019

Author Contributor

I was trying to convert some of the JS into jQuery while refactoring the code. Should I continue doing this in this PR itself or should I create a new one?

This comment has been minimized.

Copy link
@mauriciofauth

mauriciofauth Apr 8, 2019

Member

What I want to know is, for example in the #eQuery case, what are the advantages in using jQuery instead of using plain JavaScript?

This comment has been minimized.

Copy link
@Bournvita1998

Bournvita1998 Apr 8, 2019

Author Contributor

@mauriciofauth I would like to suggest that if you hadn't looked my proposal on "Refactoring and Improving Designer" till now, please have a look at that you will definitely find the answer for "what are the advantages in using jQuery instead of using plain JavaScript?"

This comment has been minimized.

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Hello. Thanks for your contribution!

We require a full name for legal compliance reasons. Could you use your full name in the signature tag?
For example: Signed-off-by: Jane Developer <jane@example.org>

See Developer's Certificate of Origin

Bournvita1998 added some commits Apr 8, 2019

Removed the linting errors
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
Improvements in move.js
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
@codecov

This comment has been minimized.

Copy link

commented Apr 10, 2019

Codecov Report

Merging #15165 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #15165      +/-   ##
============================================
+ Coverage     52.82%   52.83%   +0.01%     
  Complexity    14044    14044              
============================================
  Files           485      485              
  Lines         62139    62131       -8     
============================================
+ Hits          32826    32829       +3     
+ Misses        29313    29302      -11
@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@williamdes If everything is fine, should I go ahead with other changes?

@williamdes

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@Bournvita1998 I would say yes, @mauriciofauth do you agree ?

@Bournvita1998 Bournvita1998 changed the title refactored 2 files as of now Improving designer Apr 12, 2019

fiexs #15163
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>

Improved the performance of all the JS files of the designer and imrpoved the names of the new variables used.
@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@williamdes I am done with all the changes, please review.

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@williamdes @mauriciofauth I am thinking of closing this PR and create new PRs in a step by step manner as mentioned in my proposal. Its because I will be working on the designer project this summer. Should I go ahead?
Also, can you please review my proposal and suggest any changes if required?
https://docs.google.com/document/d/16rg0DImKFuW_dwxGXbgncJQ0NjZuglXVkRF2rUPzrwA/edit?usp=sharing

@williamdes

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@Bournvita1998 I agree, good proposal !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.