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.

Sign up for GitHub

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

Jump to bottom

Removed inline onchange #13294

Merged
merged 1 commit into from Jun 12, 2017
Merged

Removed inline onchange #13294

merged 1 commit into from Jun 12, 2017

Conversation

manishbisht
Copy link
Contributor

Fixes #12261

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests

@codecov
Copy link

codecov bot commented May 23, 2017 •
edited

Codecov Report

Merging #13294 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #13294   +/-   ##
=======================================
  Coverage   54.56%   54.56%           
=======================================
  Files         467      467           
  Lines       70465    70465           
=======================================
  Hits        38449    38449           
  Misses      32016    32016

@manishbisht
Copy link
Contributor Author

@nijel Can you please restart the build. I think there is some timeout issue.

nijel
nijel reviewed May 24, 2017
View reviewed changes
templates/table/search/column_comparison_operators.phtml Outdated
changeValueFieldType(this, <?= $search_index; ?>);
});
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

This really doesn't help for CSP - both need unsafe-inline. Can you please move that to one of js files?

@manishbisht
Copy link
Contributor Author

@nijel I have updated the PR.

@manishbisht
Copy link
Contributor Author

@nijel @ibennetch Can we merge this PR ?

@ibennetch
Copy link
Member

ibennetch commented May 29, 2017 via email

nijel
nijel suggested changes Jun 1, 2017
View reviewed changes
js/tbl_select.js Outdated
@@ -81,6 +81,13 @@ AJAX.registerOnload('tbl_select.js', function () {
return false;
});

var tableRows = $('.data tbody tr');
Copy link
Contributor

Choose a reason for hiding this comment

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

This selector looks too generic. Also it's probably better to use $('...').each(...) instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked this generic selector is actually good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It will select the data which have .data class then the tbody tag and at the last all the tr tags inside it. And make an object of the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the data class is used in more situations, won't it match too many?

Also shouldn't you rather target on select instead of tr? The tr certainly doesn't have onchange...

templates/table/search/column_comparison_operators.phtml Outdated
@@ -1,5 +1,4 @@
<select name="criteriaColumnOperators[<?= $search_index; ?>]"
onchange="changeValueFieldType(this, <?= $search_index; ?>)">
<select id="ColumnOperator<?= $search_index; ?>" name="criteriaColumnOperators[<?= $search_index; ?>]">
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I've merged migration of those templates to Twig meanwhile, can you please update your PR?

@manishbisht
Copy link
Contributor Author

@nijel I have update the PR

nijel
nijel reviewed Jun 2, 2017
View reviewed changes
js/tbl_select.js Outdated
@@ -81,6 +81,13 @@ AJAX.registerOnload('tbl_select.js', function () {
return false;
});

var tableRows = $('.data tbody tr');
$(tableRows).each(function(index, item){
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need $() around tableRows here (it's already constructed as jQuery object list).

@manishbisht
Copy link
Contributor Author

@nijel Updated the PR with the suggested changes.

@manishbisht
Copy link
Contributor Author

manishbisht commented Jun 9, 2017 •
edited

@nijel Yes, You are right select should be used instead of tr I have updated the PR.

nijel
nijel reviewed Jun 9, 2017
View reviewed changes
js/tbl_select.js Outdated
@@ -81,6 +81,13 @@ AJAX.registerOnload('tbl_select.js', function () {
return false;
});

var tableRows = $('select');
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't just match all selects on the page. Probably adding class to the one you want to match is needed. Have you tested the change that it doesn't affect something else?

@manishbisht
remove inline onchange
d5e32c3 
Signed-off-by: Manish Bisht <manish.bisht490@gmail.com>
@manishbisht
Copy link
Contributor Author

@nijel I have added extra id selector $('#fieldset_table_qbe select'); to get results more accurately. Yes I have tested the change by putting the console.log() statements before the function call and also verified that the length of tableRows should be equal to the number of columns in the table.

@nijel nijel merged commit fe0cef5 into phpmyadmin:master Jun 12, 2017
@nijel
Copy link
Contributor

nijel commented Jun 12, 2017

Merged, thanks for your contribution!

@nijel nijel added this to the 4.7.2 milestone Jun 12, 2017
@nijel nijel self-assigned this Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nijel nijel nijel approved these changes

Assignees

@nijel nijel

Labels
None yet
Projects
None yet
Milestone
4.7.2
Development

Successfully merging this pull request may close these issues.

Remove inline javascript
3 participants
@manishbisht @ibennetch @nijel