prevent jqmData calls for columns without a priority #7315
Conversation
| @@ -85,9 +85,10 @@ $.widget( "mobile.table", $.mobile.table, { | |||
| this.headers.not( "td" ).each( function() { | |||
| var header = $( this ), | |||
| priority = $.mobile.getAttribute( this, "priority" ), | |||
| cells = header.add( header.jqmData( "cells" ) ); | |||
| cells; | |||
There was a problem hiding this comment.
Variables which are not assigned should be comma separated on the first line with assigned ones each on their own line after.
|
Thanks for taking the time to contribute to jQuery Mobile. We have the following requirements of pull requests before we can review them.
If you are interested in continuing with this change, please correct the remaining items and we will look into this for you. |
|
Cs fixed, thanks. The fix does not change behavior, fix a bug or or add a new feature. Therefore I am not sure, whether a unit test or testcase is required. Its more like a trivial performance fix. |
|
In case you agree, i would squash the commits and fix the commit message |
| var header = $( this ), | ||
| priority = $.mobile.getAttribute( this, "priority" ), | ||
| cells = header.add( header.jqmData( "cells" ) ); | ||
| var header = $( this ), cells, |
There was a problem hiding this comment.
cells should be on the first line by it self see http://contribute.jquery.org/style-guide/js/#assignments
|
Normally I would like to see a check to make sure we are not assigning this if its not needed however the way the code here works there is no way to check this so no need for a unit test you are correct. We would like to still see an issue so that it can be referenced from this PR and give it a chance for people to comment on it if they would like but no test case is needed. |
|
What do you mean by "we want to see an issue"? Should I provide a jsfiddle with a patched jquery mobile version and a usage sample? Or do you mean I need to create a issue in some sort of external issue tracker? |
|
@staabm Please file an issue at https://github.com/jquery/jquery-mobile/issues/new, then modify the commit message to end with the line where |
|
Not sure why we need another issue, but here it is I will adjust the commit message when I am back at my pc tomorrow |
|
@staabm Thanks a lot! |
|
Commits squashed and message adjusted. Hopefully I matched your commit style :-). |
|
It still needs a unit test to make sure that |
No description provided.