Selectmenu: Add classes option #8309

Closed
wants to merge 22 commits into
from

Projects

None yet

5 participants

@gabrielschulhof
Contributor

No description provided.

@gabrielschulhof gabrielschulhof self-assigned this Oct 17, 2015
@gabrielschulhof gabrielschulhof added this to the 1.5.0 milestone Oct 17, 2015
@arschmitz
Member

@gabrielschulhof were you wanting a review of this

@gabrielschulhof
Contributor

@arschmitz I've made a few more changes and I could use a review although I'm fairly certain it's not ready to go yet, but it'll be good to have another set of eyes on it so I have a clear list of what remains to be done. TIA!

@arschmitz arschmitz commented on an outdated diff Nov 19, 2015
js/widgets/forms/select.custom.js
@@ -79,16 +94,24 @@ return $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
}
if ( event.type === "vclick" ||
- event.keyCode && ( event.keyCode === $.mobile.keyCode.ENTER || event.keyCode === $.mobile.keyCode.SPACE ) ) {
+ event.keyCode &&
+ ( event.keyCode === $.mobile.keyCode.ENTER ||
@arschmitz
arschmitz Nov 19, 2015 Member

use $.ui.keyCode this is deprecated

@arschmitz arschmitz commented on the diff Nov 19, 2015
js/widgets/forms/select.custom.js
this._decideFormat();
if ( this.menuType === "overlay" ) {
- this.button.attr( "href", "#" + this.popupId ).attr( "data-" + ( $.mobile.ns || "" ) + "rel", "popup" );
+ this.button
+ .attr( "href", "#" + this.popupId )
@arschmitz
arschmitz Nov 19, 2015 Member

is popupId escaped elsewhere?

@gabrielschulhof
gabrielschulhof Dec 22, 2015 Contributor

No, and it doesn't need to be, because I'm not constructing a selector with it.

@arschmitz arschmitz commented on an outdated diff Nov 19, 2015
js/widgets/forms/select.custom.js
this._decideFormat();
if ( this.menuType === "overlay" ) {
- this.button.attr( "href", "#" + this.popupId ).attr( "data-" + ( $.mobile.ns || "" ) + "rel", "popup" );
+ this.button
+ .attr( "href", "#" + this.popupId )
+ .attr( "data-" + ( $.mobile.ns || "" ) + "rel", "popup" );
@arschmitz
arschmitz Nov 19, 2015 Member

ns is deprecated move determining this to backcompat and make an extension point

@arschmitz arschmitz commented on an outdated diff Nov 19, 2015
js/widgets/forms/select.custom.js
} else {
- this.button.attr( "href", "#" + this.dialogId ).attr( "data-" + ( $.mobile.ns || "" ) + "rel", "dialog" );
+ this.button
+ .attr( "href", "#" + this.dialogId )
@arschmitz
arschmitz Nov 19, 2015 Member

same as above is this escaped

@arschmitz arschmitz commented on an outdated diff Nov 19, 2015
js/widgets/forms/select.custom.js
} else {
- this.button.attr( "href", "#" + this.dialogId ).attr( "data-" + ( $.mobile.ns || "" ) + "rel", "dialog" );
+ this.button
+ .attr( "href", "#" + this.dialogId )
+ .attr( "data-" + ( $.mobile.ns || "" ) + "rel", "dialog" );
@arschmitz
arschmitz Nov 19, 2015 Member

move to backcompat

@arschmitz arschmitz commented on an outdated diff Nov 19, 2015
js/widgets/forms/select.custom.js
@@ -233,33 +257,40 @@ return $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
"overlay-theme='" + overlayTheme + "'" ) : "";
dividerThemeAttr = ( o.dividerTheme && this.element.children( "optgroup" ).length > 0 ) ?
( " data-" + $.mobile.ns + "divider-theme='" + o.dividerTheme + "'" ) : "";
- menuPage = $( "<div data-" + $.mobile.ns + "role='dialog' class='ui-selectmenu'" +
- themeAttr + overlayThemeAttr + ">" +
- "<div data-" + $.mobile.ns + "type='header'>" +
- "<div class='ui-title'></div>" +
- "</div>" +
- "<div class='ui-content'></div>" +
+ menuPage = $( "<div data-" + $.mobile.ns + "role='dialog'>" +
@arschmitz
arschmitz Nov 19, 2015 Member

move to backcompat

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.custom.js
// Custom selects cannot exist inside popups, so revert the "nativeMenu" option to true if
// a parent is a popup
- o.nativeMenu = o.nativeMenu || ( this.element.parents( ":jqmData(role='popup'),:mobile-popup" ).length > 0 );
+ o.nativeMenu = o.nativeMenu ||
+ ( this.element.parents( ":jqmData(role='popup'),:mobile-popup" ).length > 0 );
@arschmitz
arschmitz Dec 3, 2015 Member

dont use jqmdata its deprecated.

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.custom.js
_decideFormat: function() {
- var self = this,
- $window = this.window,
- selfListParent = self.list.parent(),
+ var $window = this.window,
@arschmitz
arschmitz Dec 3, 2015 Member

remove hungarian notation

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.custom.js
// For WebOS/Opera Mini (set lastscroll using button offset)
if ( scrollTop === 0 && buttonOffset > screenHeight ) {
- self.thisPage.one( "pagehide", function() {
@arschmitz
arschmitz Dec 3, 2015 Member

pagehide is deprecated

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.custom.js
$( this ).jqmData( "lastScroll", buttonOffset );
} );
}
- self.menuPage.one( {
+ this.menuPage.one( {
pageshow: $.proxy( this, "_focusMenuItem" ),
pagehide: $.proxy( this, "close" )
@arschmitz
arschmitz Dec 3, 2015 Member

use page container events

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.custom.js
@@ -523,7 +598,10 @@ return $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
}
}
- if ( needPlaceholder && ( !option.getAttribute( "value" ) || text.length === 0 || $option.jqmData( "placeholder" ) ) ) {
+ if ( needPlaceholder &&
+ ( !option.getAttribute( "value" ) ||
+ text.length === 0 ||
+ $option.jqmData( "placeholder" ) ) ) {
@arschmitz
arschmitz Dec 3, 2015 Member

dont use jqmdata

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.js
@@ -320,4 +334,6 @@ return $.widget( "mobile.selectmenu", $.extend( {
}
}, $.mobile.behaviors.formReset ) );
@arschmitz
arschmitz Dec 3, 2015 Member

use UI form reset and also $.widget now supports mixins via passing an array

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.js
_preExtension: function() {
- var inline = this.options.inline || this.element.jqmData( "inline" ),
- mini = this.options.mini || this.element.jqmData( "mini" ),
- classes = "";
- // TODO: Post 1.1--once we have time to test thoroughly--any classes manually applied to the original element should be carried over to the enhanced element, with an `-enhanced` suffix. See https://github.com/jquery/jquery-mobile/issues/3577
- /* if ( $el[0].className.length ) {
- classes = $el[0].className;
- } */
+ var classes = "";
+
+ // TODO: Post 1.1--once we have time to test thoroughly--any classes manually applied to
+ // the original element should be carried over to the enhanced element, with an `-enhanced`
+ // suffix. See https://github.com/jquery/jquery-mobile/issues/3577
+ // if ( $el[0].className.length ) {
+ // classes = $el[0].className;
+ // }
@arschmitz
arschmitz Dec 3, 2015 Member

this is no longer relavent

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.js
@@ -95,49 +100,54 @@ return $.widget( "mobile.selectmenu", $.extend( {
classes = " ui-button-right";
}
- if ( inline ) {
- classes += " ui-button-inline";
- }
- if ( mini ) {
- classes += " ui-mini";
- }
+ this._removeClass( this.element, null, "ui-button-left ui-button-right" );
+ this.select = this.element;
@arschmitz
arschmitz Dec 3, 2015 Member

whats the point of this.select if its always this.element?

@arschmitz arschmitz commented on an outdated diff Dec 3, 2015
js/widgets/forms/select.js
}
},
_create: function() {
+ var options = this.options,
+ iconpos = options.icon ?
+ ( options.iconpos || this.select.jqmData( "iconpos" ) ) : false;
@arschmitz
arschmitz Dec 3, 2015 Member

dont use jqmData

@arschmitz
Member

Looking good done for now

@arschmitz arschmitz commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.custom.js
@@ -51,14 +53,32 @@ var unfocusableItemSelector = ".ui-disabled,.ui-state-disabled,.ui-listview-item
};
return $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
+ options: {
+ classes: {
+ "ui-selectmenu-custom-header-close-button":
+ "ui-button ui-corner-all ui-button-left ui-button-icon-only ui-icon-delete"
@arschmitz
arschmitz Feb 18, 2016 Member

ui-button* is not a theme class neither is ui-icon*

@arschmitz arschmitz commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.custom.js
this.buttonId + "'" + themeAttr + dividerThemeAttr + "></ul>" )
.attr( "id", menuId )
.appendTo( listbox );
- header = $( "<div class='ui-header ui-bar-" + ( o.theme ? o.theme : "inherit" ) + "'></div>" ).prependTo( listbox );
- headerTitle = $( "<h1 class='ui-title'></h1>" ).appendTo( header );
+ header = $( "<div>" )
+ .toolbar( { type: "header" } )
+ .prependTo( listbox );
+ headerTitle = $( "<h1></h1>" ).appendTo( header );
+
+ this._addClass( menuPage, null, "ui-selectmenu-custom" );
@arschmitz
arschmitz Feb 18, 2016 Member

ui-selectmenu* should be in the second param they belong to this widget

@arschmitz arschmitz and 1 other commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.custom.js
if ( this.isMultiple ) {
headerClose = $( "<a>", {
"role": "button",
"text": o.closeText,
- "href": "#",
- "class": "ui-button ui-corner-all ui-button-left ui-button-icon-only ui-icon-delete"
- } ).appendTo( header );
+ "href": "#"
+ } );
+ this._addClass( headerClose, "ui-selectmenu-custom-header-close-button" );
@arschmitz
arschmitz Feb 18, 2016 Member

same here

@gabrielschulhof
gabrielschulhof Mar 15, 2016 Contributor

I don't get it. ui-selectmenu-custom-header-close-button is in the second param here.

@arschmitz arschmitz and 1 other commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.js
},
_destroy: function() {
- var wrapper = this.element.parents( ".ui-select" );
- if ( wrapper.length > 0 ) {
- if ( wrapper.is( ".ui-button-left, .ui-button-right" ) ) {
- this.element.addClass( wrapper.hasClass( "ui-button-left" ) ? "ui-button-left" : "ui-button-right" );
+ if ( this.selectWrapper.length > 0 ) {
+ if ( this.selectWrapper.is( ".ui-button-left, .ui-button-right" ) ) {
+ this._addClass( null,
@arschmitz
arschmitz Feb 18, 2016 Member

button classes should be in the thuird param

@arschmitz
arschmitz Feb 18, 2016 Member

also no need to remove classes in destroy

@arschmitz
arschmitz Feb 18, 2016 Member

no need to do anything you remove the whole element below

@gabrielschulhof
gabrielschulhof Mar 15, 2016 Contributor

This adds classes back to this.element upon destroy.

@gabrielschulhof
gabrielschulhof Mar 15, 2016 Contributor

... and if you leave out the element to which you wish to apply the classes, it becomes this.element, so the classes are in the correct parameter, AFAICT.

@gabrielschulhof
gabrielschulhof Mar 16, 2016 Contributor

Wait a sec - if I use this._addClass() here, classes will be added but then immediately removed, right? Hmmm ...

@arschmitz arschmitz commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.js
},
_create: function() {
+ var options = this.options,
+ iconpos = options.icon ?
+ ( options.iconpos || this.element.jqmData( "iconpos" ) ) : false;
@arschmitz
arschmitz Feb 18, 2016 Member

dont use jqmdata

@arschmitz arschmitz commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.js
@@ -214,7 +221,7 @@ return $.widget( "mobile.selectmenu", $.extend( {
$.mobile.zoom.disable( true );
}
} );
- self.select.bind( "focus", function() {
+ self.element.bind( "focus", function() {
@arschmitz
arschmitz Feb 18, 2016 Member

should really avoid self since in some contexts in can reference window

@arschmitz arschmitz commented on an outdated diff Feb 18, 2016
js/widgets/forms/select.js
span.attr( "aria-hidden", "true" );
// TODO possibly aggregate multiple select option classes
return span
- .addClass( self.select.attr( "class" ) )
+ .addClass( self.element.attr( "class" ) )
@arschmitz
arschmitz Feb 18, 2016 Member

using addClass still

@arschmitz
Member

looking good only a few things

@gabrielschulhof gabrielschulhof changed the title from WIP: Selectmenu: Add classes option to Selectmenu: Add classes option Mar 15, 2016
@gabrielschulhof
Contributor

@jaspermdegroot can you please check http://view.jquerymobile.com/7731-selectmenu-classes-option/demos/selectmenu-custom/ and tell me if I'm doing anything wrong with the classes? The styling is obviously wrong, but is it wrong because I'm adding/failing to add classes wrong, or is it wrong because the contents of the CSS is wrong?

@gabrielschulhof
Contributor
@gabrielschulhof
Contributor

@jaspermdegroot reminder: 9743d32 removes the fix for #5073, but you said it should be fixed in CSS.

@jaspermdegroot
Member

@gabrielschulhof @arschmitz

This test page shows the issue from #5073: http://jsbin.com/zikohihiyu/edit?html,output

I can add CSS to negate the rules from ui-toolbar-header-button-left/right when the class is on a span inside a custom select menu, but then the user still doesn't get the header left/right button positioning that (s)he wants. When that ticket was opened we were automatically adding the left/right button classes but we don't do that anymore, so I think it's best to document / demonstrate that in case of a select you have to wrap it in a div and add the button left/right class to that div, never on the select itself.

What did we decide about how we handle classes in case of generated wrappers? To add a class to the container you need to use the classes option (or enhance the selectmenu yourself once we added that option)? Adding a class to the select itself is not supported?

@arschmitz
Member

@jaspermdegroot classes option would be the only fuly supported way to do that

@gabrielschulhof
Contributor

@jaspermdegroot could you please have a look at the link below?

http://jsbin.com/jujobam/1/

It is based on this PR.

Even though the classes ui-mini and ui-toolbar-header-button-right are getting applied to the selectmenu wrapper, position: absolute; from ui-toolbar-header-button-right is getting overridden by position: relative; from ui-selectmenu because the latter seems to have precedence.

What is the best way to resolve this?

There also seems to be a problem with the arrow icon in the button. Can you tell me what I'm doing wrong?

@jaspermdegroot
Member

@gabrielschulhof

Even though the classes ui-mini and ui-toolbar-header-button-right are getting applied to the selectmenu wrapper, position: absolute; from ui-toolbar-header-button-right is getting overridden by position: relative; from ui-selectmenu because the latter seems to have precedence.

I think we can remove position: relative; for ui-selectmenu. The select element has position: absolute; so one of the wrapper divs should get position: relative; (or anything other than the default static). ui-button already has relative so I think it's safe to remove it from the outer wrapper.

Have to check with a custom selectmenu as well. I can't preview this branch on view.* since it's in your fork. Just see if anything breaks if you remove it. I can always double check after this has been merged.

There also seems to be a problem with the arrow icon in the button. Can you tell me what I'm doing wrong?

I suppose you mean the lack of space between text and icon? Since we make the icon float with class ui-widget-icon-floatend we will have to add extra padding to the button. Maybe we should do that in core/button CSS, not in selectmenu CSS. Will look into that.

@gabrielschulhof gabrielschulhof added a commit to gabrielschulhof/jquery-mobile that referenced this pull request Apr 7, 2016
@gabrielschulhof gabrielschulhof Selectmenu: Remove relative positioning de6b6a8
@gabrielschulhof
Contributor

@jaspermdegroot

Thanks for looking into it! I've removed position: relative; from ui-selectmenu and it's all good now.

Meanwhile, here's a bin illustrating both custom and non-custom select in both a toolbar and non-toolbar setting. The bin should look right once view updates the branch.

http://jsbin.com/zukase/1

I've pushed this branch upstream as well. http://view.jquerymobile.com/7731-selectmenu-classes-option/demos/

@gabrielschulhof
Contributor

@jaspermdegroot the problem with the arrow icon on the selectmenu is actually pretty bad on Firefox:

7731-firefox

@gabrielschulhof
Contributor

OK, so if the icon span precedes the label span then FF looks OK. Probably because we never set the top and it's absolutely positioned and in FF, the "cursor" wraps to the next line, whereas in Chrome it does not.

@gabrielschulhof
Contributor

http://output.jsbin.com/zukase/ fetches the CSS from rawgit.com, so that shows the updated CSS, but unfortunately, I can't show the updated JS because only view can give me that service, and view doesn't seem to be updating.

@gabrielschulhof
Contributor

Yaaay! view has updated! http://jsbin.com/zukase/1 now shows both the removal of position: absolute; and the effect of having rearranged the spans such that the icon comes first. Works in both FF and Chrome.

@gabrielschulhof
Contributor

I think this is ripe for another round of review by both @jaspermdegroot and @arschmitz.

@jaspermdegroot
Member

The lack of space between text and icon should be fixed with this PR: #8426

@jaspermdegroot
Member

CSS looks good. There are problems with the custom selectmenu popup on FF though.

@arschmitz arschmitz commented on an outdated diff Apr 21, 2016
external/qunit-assert-classes/qunit-assert-classes.js
@@ -58,8 +58,8 @@
}
return {
- missing: missing,
- found: found,
+ missing: missing || [],
+ found: found || [],
@arschmitz
arschmitz Apr 21, 2016 edited Member

this cant be changed directly this is an external dependency. If there is a bug you should file an issue.

@arschmitz arschmitz commented on an outdated diff Apr 21, 2016
js/widgets/forms/select.js
} else {
// Browser globals
factory( jQuery );
}
} )( function( $ ) {
-return $.widget( "mobile.selectmenu", $.extend( {
+return $.widget( "mobile.selectmenu", $.widget( "mobile.selectmenu", [ {
@arschmitz
arschmitz Apr 21, 2016 Member

this call is weird i think it would be more clear to separate out adding the theme

@arschmitz
Member

Looks mostly good just those 2 things plus what @jaspermdegroot mentioned

gabrielschulhof added some commits Feb 3, 2016
@gabrielschulhof gabrielschulhof Dependencies: add jquery-ui/form-reset-mixin.js 98b8873
@gabrielschulhof gabrielschulhof Page: Use _.on() for pagehide a9c7691
@gabrielschulhof gabrielschulhof Selectmenu: Implement classes option
Fixes gh-7731
ff37ac3
@gabrielschulhof gabrielschulhof Selectmenu: Update tests to use bootstrap 8201e75
@gabrielschulhof gabrielschulhof Selectmenu: Fix assumptions in option test 188ccb0
@gabrielschulhof gabrielschulhof Selectmenu: Remove relative positioning 63a00ba
@gabrielschulhof gabrielschulhof Selectmenu: Icon span must precede text span 0b1a992
@gabrielschulhof gabrielschulhof Selectmenu: Add class key to button text span f7b7123
@gabrielschulhof gabrielschulhof Selectmenu: Update toolbar-related class names 1410f30
@gabrielschulhof gabrielschulhof Selectmenu: Fix popup header close button style a6cb1d4
@gabrielschulhof gabrielschulhof Selectmenu: Create toolbars late to make sure they find containing page f6ea626
@gabrielschulhof gabrielschulhof Core: Add ui- namespace to page selectors 1cf5972
@gabrielschulhof gabrielschulhof Selectmenu: Address review comments 5aa92fa
@gabrielschulhof gabrielschulhof Widget: Define $.mobile.widget.backcompat in all cases
$.mobile.widget.backcompat must not be undefined even if $.mobileBackcompat is
false, since other widgets unconditionally extend with
$.mobile.widget.backcompat and if $.mobile.widget.backcompat is undefined, then
they end up doing $.widget( ..., undefined ) which results in _create() being
assigned $.noop.
299f661
@gabrielschulhof gabrielschulhof Selectmenu: Remove list from dialog only after it is completely hidden 016b8c4
@gabrielschulhof gabrielschulhof Button: Correct spelling error a7cec66
@gabrielschulhof gabrielschulhof Selectmenu: Fix controlgroup interaction adfc277
@gabrielschulhof gabrielschulhof Controlgroup: Fix themeing 0b9326a
@gabrielschulhof gabrielschulhof All: Address review comments 12aa018
@gabrielschulhof gabrielschulhof Tests: Add define() wrappers
33a5648
@gabrielschulhof gabrielschulhof All: Remove UI core and references to it
71e7dfb
@gabrielschulhof
Contributor

@arschmitz we have a load order race condition in kitchensink which seems to reliably manifest itself with core 1.9.1 in this PR. Aside from that it should be golden. I'll try to look into it whenever I get a chance.

@gabrielschulhof gabrielschulhof Tests: Use $.Deferred() in kitchensink
9fbda32
@gabrielschulhof
Contributor

@arschmitz nm, using a $.Deferred(). Lessee...

@gabrielschulhof
Contributor

Yaaaay! Tests are now no worse than 1.5-dev!

@arschmitz
Member

👍

@gabrielschulhof gabrielschulhof added a commit that referenced this pull request Jun 23, 2016
@gabrielschulhof gabrielschulhof Selectmenu: Implement classes option
Fixes gh-7731
Closes gh-8309
fdbe5c7
@apsdehal
Member
apsdehal commented Jun 23, 2016 edited

Awesome! @gabrielschulhof landed this in 1.5-dev.

@apsdehal apsdehal closed this Jun 23, 2016
@arschmitz arschmitz added a commit to arschmitz/jquery-mobile that referenced this pull request Jul 4, 2016
@gabrielschulhof @arschmitz gabrielschulhof + arschmitz Selectmenu: Implement classes option
Fixes gh-7731
Closes gh-8309
cecafcf
@arschmitz arschmitz added a commit that referenced this pull request Jul 4, 2016
@gabrielschulhof @arschmitz gabrielschulhof + arschmitz Selectmenu: Implement classes option
Fixes gh-7731
Closes gh-8309
649b09f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment