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

Page: Implement classes option #8121

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

commented Apr 20, 2015

Fixes gh-7707

@@ -80,12 +80,12 @@ return $.widget( "mobile.dialog", {
opts = this.options;

// Class the markup for dialog styling and wrap interior
elem.addClass( "ui-dialog" )
elem.addClass( "ui-page-dialog" )

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

shouldnt this be using _addClass? same below for the wrap

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof May 2, 2015

Author Contributor

The dialog widget is deprecated. I just renamed the classes, that's all, so I had to touch this file for the deprecated widget to continue to look OK, but I wasn't intent on implementing the classes option for it. Should I?

if ( o.contentTheme !== undefined ) {
this.element.find( "[data-" + $.mobile.ns + "='content']" ).removeClass( "ui-body-" + this.options.contentTheme )
.addClass( "ui-body-" + o.contentTheme );
_setOptions: function( newOptions ) {

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

since we are changing this whole function anyway and it really makes no sense for a single option the way it is, lets switch this to follow the UI convention of setOption vs setOptions


$( "#close-button-test" ).page( "option", "closeBtn", "left" );
deepEqual( $( "#close-button-test .ui-header a" ).length, 0,

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

deepEqual makes no sense here your comparing integers. Also even comparing integers seems unneeded just do

ok( !$( "#close-button-test .ui-header a" ).length,
    "Initially, the dialog header has no anchor elements (option value 'none')" );
a = $( "#close-button-test .ui-header a" );
deepEqual( a.length, 1, "The dialog header has eactly one anchor element when the option value is set to 'left'" );
ok( a.hasClass( "ui-button-left" ), "The close button has class ui-button-left when the closeBtn option is set to 'left'" );
$( "#close-button-test" ).page( "option", "closeBtn", "right" );
assert.hasClasses( a[ 0 ], "ui-button-left",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

why are you doing a[ 0 ] not just a?

},

function() {
assert.deepEqual( $.mobile.activePage.attr( "id" ), "overlayTheme-test",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

Do not use deepEqual here its only for arrays, objects, regular expressions, dates and functions http://api.qunitjs.com/deepEqual/

assert.hasClasses( a[ 0 ], "ui-button-right",
"The close button has class ui-button-right when the closeBtn option is set " +
"to 'right'" );
deepEqual( a.text(), "Custom text", "Anchor text successfully set via option" );

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

don't use deepEqual

dialog.page( "option", {
"closeBtn": "right",
"closeBtnText": "Custom text"
} );
a = $( "#close-button-test .ui-header a" );
deepEqual( a.length, 1, "The dialog header has eactly one anchor element when the option value is set to 'right'" );

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

don't use deepEqual

deepEqual( $( "#close-button-test .ui-header a" ).length, 0,
"Initially, the dialog header has no anchor elements (option value 'none')" );

dialog.page( "option", "closeBtn", "left" );
a = $( "#close-button-test .ui-header a" );
deepEqual( a.length, 1, "The dialog header has eactly one anchor element when the option value is set to 'left'" );

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

don't use deepEqual

deepEqual( a.text(), "Custom text", "Anchor text successfully set via option" );

dialog.page( "option", "closeBtn", "none" );
deepEqual( $( "#close-button-test .ui-header a" ).length, 0,

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

don't use deepEqual

ok( dialog.find( ":jqmData(role=footer)" ).hasClass( "ui-bar-inherit" ), "Expected footer to inherit from dialog" );
assert.hasClasses( dialog[ 0 ], "ui-page-theme-a",
"Expected explicit theme ui-page-theme-a" );
assert.hasClasses( $( "body" )[ 0 ], "ui-overlay-a",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

Why are you passing the native element?

ok( dialog.find( ":jqmData(role=header)" ).hasClass( "ui-bar-inherit" ), "Expected header to inherit from dialog" );
ok( dialog.find( ":jqmData(role=content)" ).hasClass( "ui-body-" + $.mobile.page.prototype.options.contentTheme ), "Expect content to inherit from $.mobile.page.prototype.options.contentTheme" );
ok( dialog.find( ":jqmData(role=footer)" ).hasClass( "ui-bar-inherit" ), "Expected footer to inherit from dialog" );
assert.hasClasses( dialog[ 0 ], "ui-page-theme-a",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

Why are you passing the native element?

"Expected explicit theme ui-page-theme-a" );
assert.hasClasses( $( "body" )[ 0 ], "ui-overlay-a",
"Expected default overlay theme ui-overlay-a" );
assert.hasClasses( dialog.find( ":jqmData(role=header)" )[ 0 ], "ui-bar-inherit",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

All of the classes asserts allow jquery objects there is no need to pass the native elements.

"Expected explicit theme ui-page-theme-e" );
assert.lacksClasses( $( "body" )[ 0 ], "ui-overlay-b",
"Expected no overlay theme ui-overlay-b" );
assert.hasClasses( $( "body" )[ 0 ], "ui-overlay-a",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

instead of lacks followed by has just use hasClassesStrict

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof May 5, 2015

Author Contributor

I cannot replace the last two assertions by a single .hasClassesStrict() because the page container may or may not have additional classes such as ui-viewport added by the page container widget. Ascertaining the presence/absence of that class is beyond the scope of this dialog-related test.

$( "#open-enhanced-dialog" ).click();
},
function() {
assert.deepEqual( $.mobile.activePage.attr( "id" ), "enhanced-dialog",

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

don't use deepEqual

@@ -1273,7 +1273,8 @@ asyncTest( "prefetched links with data rel dialog result in a dialog", function(
},

function() {
ok( $.mobile.activePage.is( ".ui-dialog" ), "prefetched page is rendered as a dialog" );
ok( $.mobile.activePage.is( ".ui-page-dialog" ),

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

this should fit on one like it looks like

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

this should be using hasClasses

@@ -27,7 +27,7 @@ asyncTest( "dialog sized select should alter the value of its parent select", fu
},

function() {
ok( $.mobile.activePage.hasClass( 'ui-dialog' ), "the dialog came up" );
ok( $.mobile.activePage.hasClass( 'ui-page-dialog' ), "the dialog came up" );

This comment has been minimized.

Copy link
@arschmitz

arschmitz Apr 30, 2015

Member

this should be using hasClasses

@arschmitz

This comment has been minimized.

Copy link
Member

commented Apr 30, 2015

I stopped commenting on everything but lots of a few repeated problems

  • still using hasClass and is in a lot of tests even ones you are updating for class name changes
  • Lots of incorrect uses of deepEqual
  • Most if not all of the classes asserts unnecessarily passing native DOM node instead of jquery objects
  • Don't compare length to 0 just do !thing.length
  • Use hasClassesStrict instead of hasClasses followed by lacksClasses on same element
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2015

OK, I'll change .deepEqual() to .strictEqual(), but the brief description on the API doc page for .deepEqual() includes "primitive types" in the list of types on which .deepEqual() works, and integers and strings are primitive types.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2015

@arschmitz can I leave the deprecated dialog widget tests with .hasClass() et. al. and the dialog widget with .addClass(), or should I put the dialog widget through the whole classes implementation shebang, even though it's deprecated?

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:7707-page-classes-option branch from 2a1c1ee to 1864e5e May 5, 2015

@arschmitz

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

👍

gabrielschulhof added a commit that referenced this pull request Aug 4, 2015

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:7707-page-classes-option branch Aug 4, 2015

arschmitz added a commit to arschmitz/jquery-mobile that referenced this pull request Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.