Panel: Implement classes option #8350

Closed
wants to merge 7 commits into
from

Projects

None yet

4 participants

@apsdehal
Member

Fixes #7709

apsdehal added some commits Dec 19, 2015
@apsdehal apsdehal Panel: Use _ alternatives for classes methods cc48e46
@apsdehal apsdehal Panel: Implement classes option
da814c5
apsdehal added some commits Dec 22, 2015
@apsdehal apsdehal Panel: Add calls to _superApply
533ff8f
@apsdehal apsdehal Panel: Fix grunt tasks for panel, add jscs 02637e1
@apsdehal apsdehal Panel: Shift tests to use qunit-assert-classes
- Removes extra tests no longer required
70fe93f
@gabrielschulhof gabrielschulhof and 1 other commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
return $.widget( "mobile.panel", {
version: "@VERSION",
options: {
classes: {
- panel: "ui-panel",
- panelOpen: "ui-panel-open",
- panelClosed: "ui-panel-closed",
- panelFixed: "ui-panel-fixed",
- panelInner: "ui-panel-inner",
- modal: "ui-panel-dismiss",
- modalOpen: "ui-panel-dismiss-open",
- pageContainer: "ui-panel-page-container",
- pageWrapper: "ui-panel-wrapper",
- pageFixedToolbar: "ui-panel-fixed-toolbar",
- pageContentPrefix: "ui-panel-page-content", /* Used for wrapper and fixed toolbars position, display and open classes. */
- animate: "ui-panel-animate"
+ "ui-panel": "ui-panel-closed"
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

@arschmitz can correct me if I'm wrong, but I don't believe ui-panel-closed is supposed to be a value, but rather a key. The value of a class key is supposed to be a set of style-related classes, IINM.

@apsdehal
apsdehal Dec 22, 2015 Member

I suppose then there is not even a need of classes option here.

@gabrielschulhof gabrielschulhof commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
if ( panelInner.length === 0 ) {
panelInner = this._safelyWrap( this.element,
- "<div class='" + this.options.classes.panelInner + "'></div>",
+ "<div class='ui-panel-inner'></div>",
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

You have to avoid using .wrap() and .wrapAll() if the wrapper has a class added at runtime, because those methods create elements on the fly. So, the elements-created-on-the-fly will not have tracking within the widget factory because they were never passed to this._addClass(). You need to do the wrapping manually. For example,

var x = $( "<div>" );
this._addClass( x, "ui-panel-inner" );
this.element.before( x );
x.append( this.element );
@gabrielschulhof gabrielschulhof and 1 other commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
@@ -121,7 +121,7 @@ return $.widget( "mobile.panel", {
var self = this,
target = self._parentPage ? self._parentPage.parent() : self.element.parent();
- self._modal = $( "<div class='" + self.options.classes.modal + "'></div>" )
+ self._modal = $( "<div class='ui-panel-dismiss'></div>" )
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

Use

self._modal = $( "<div>" );
self._addClass( self._modal, "ui-panel-dismiss" );
@arschmitz
arschmitz Dec 22, 2015 Member

except don't use self that is also a keyword for window

@gabrielschulhof gabrielschulhof commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
if ( wrapper.length === 0 ) {
thePage = this._page();
wrapper = this._safelyWrap( thePage,
- "<div class='" + this.options.classes.pageWrapper + "'></div>",
+ "<div class='ui-panel-wrapper'></div>",
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

Same as above - use this._addClass(...)

@gabrielschulhof gabrielschulhof commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
@@ -268,9 +266,9 @@ return $.widget( "mobile.panel", {
e.preventDefault();
link = $( e.target );
if ( link.hasClass( "ui-button" ) ) {
- link.addClass( "ui-button-active" );
+ this._addClass( link, "ui-button-active" );
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

This should be this._addClass( link, null, "ui-button-active" )

@gabrielschulhof gabrielschulhof commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
this.element.one( "panelopen panelclose", function() {
- link.removeClass( "ui-button-active" );
+ this._removeClass( "ui-button-active" );
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

this._removeClass( link, null, "ui-button-active" )

@gabrielschulhof gabrielschulhof and 1 other commented on an outdated diff Dec 22, 2015
js/widgets/panel.js
@@ -360,29 +361,34 @@ return $.widget( "mobile.panel", {
}
if ( o.theme && o.display !== "overlay" ) {
- self._page().parent()
- .addClass( o.classes.pageContainer + "-themed " + o.classes.pageContainer + "-" + o.theme );
+ self._addClass( self._page().parent(),
+ classesMap.pageContainer + "-themed " +
+ classesMap.pageContainer + "-" + o.theme );
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

Why do we need the classesMap here or at all, for that matter (just curious)?

@apsdehal
apsdehal Dec 22, 2015 Member

I added to the increase code maintainability since the same string was being used all over the place. Replacing the value of the variable replaces the value at every place where the variable is used.

@gabrielschulhof gabrielschulhof and 1 other commented on an outdated diff Dec 22, 2015
tests/unit/panel/panel_core.js
originalWidget = $.mobile.panel.prototype;
+defaults.classes = classes;
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

Huh? Why do we need all this? I thought we were dropping these class keys.

@arschmitz
arschmitz Dec 22, 2015 Member

that was the plan

@gabrielschulhof gabrielschulhof commented on an outdated diff Dec 22, 2015
tests/unit/panel/panel_core.js
- assert.equal( $panel.hasClass( defaults.classes.animate ), $.support.cssTransform3d, "animate class is present by default when supported" );
- assert.ok( $panel.hasClass( defaults.classes.panelClosed ), "panel is closed by default" );
+ assert.hasClasses( $panel, defaults.classes.panelClosed,
@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

Could just use ui-panel-closed here.

@apsdehal apsdehal Panel: Address review comments
6245128
@arschmitz arschmitz commented on an outdated diff Jan 28, 2016
js/widgets/panel.js
- // if animating, add the class to do so
+ this._addClass( this.element, "ui-panel ui-panel-closed", this._getPanelClasses() );
@arschmitz
arschmitz Jan 28, 2016 Member

no need to pass this.element its the default, this._addClass( "ui-panel ui-panel-closed", this._getPanelClasses() );

@arschmitz arschmitz commented on an outdated diff Jan 28, 2016
js/widgets/panel.js
if ( $.support.cssTransform3d && !!this.options.animate ) {
- this.element.addClass( this.options.classes.animate );
+ this._addClass( this.element, "ui-panel-animate" );
@arschmitz
arschmitz Jan 28, 2016 Member

same here

@arschmitz
Member

this looks good once you make the 2 changes

@apsdehal apsdehal Panel: Address review comments
d951013
@apsdehal
Member

@arschmitz Done!

@arschmitz
Member

👍

@apsdehal apsdehal added a commit that referenced this pull request Feb 25, 2016
@apsdehal apsdehal Panel: Implement classes option
Closes gh-8350
09ab81b
@apsdehal apsdehal closed this Feb 25, 2016
@apsdehal apsdehal added a commit to apsdehal/jquery-mobile that referenced this pull request Mar 26, 2016
@apsdehal apsdehal Panel: Implement classes option
Closes gh-8350
b621f83
@arschmitz arschmitz added a commit to arschmitz/jquery-mobile that referenced this pull request Jul 4, 2016
@apsdehal @arschmitz apsdehal + arschmitz Panel: Implement classes option
Closes gh-8350
8de2977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment