Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Listview: Added option for the default list icon #5018

Merged
merged 5 commits into from Oct 9, 2012
Merged

Conversation

jasondscott
Copy link
Contributor

No description provided.

Jason Scott added 2 commits September 17, 2012 17:05
Added an option for the default list icon, instead of being hard-coded
to 'arrow-r'
@jaspermdegroot
Copy link
Contributor

hi @jasondscott !

@MauriceG and I already had a nice discussion about making it possible to set a default icon for listviews. See #4582.

I think we should keep things simple for users and not add new options if not really necessary. So I suggest to stick to one option "icon" with the following possibilities:

default = "arrow-r"

  1. [global] override the default for all listviews by using the $.mobile object
  2. [UL/OL] override the default or global with data-icon on a listview OR with the option icon if that listview is created programmatically
  3. [LI] override any of the above for a list item with data-icon on that specific list item

This could be done with the following additions / changes:

$.widget( "mobile.listview", $.mobile.widget, {
    options: {
        icon: "arrow-r",
    },


    refresh: function( create ) {
        var ...,
            listIcon = $list.jqmData( "icon" ) || o.icon,


            if ( a.length && !isDivider ) {
                    var itemIcon = item.jqmData( "icon" );   /* replaces: icon = item.jqmData( "icon" ); */

                    if ( a.length > 1 ) {
                        icon = false;
                    } else {
                        icon = itemIcon || listIcon;
                    }

                    item.buttonMarkup({
                        icon: icon,
                    });

@toddparker
Copy link
Contributor

Thanks for the PR @jasondscott. Been hashing this out in IRC and I agree that the best approach would to re-use the data-icon attribute and just allow it to be set at the UL/OL level as the default for that instance. If you set a data-icon attribute on the LIs, that would override the icon set ont he UL/OL. I think this is what @uGoMobi was suggesting in #2, but wanted to confirm.

@jaspermdegroot
Copy link
Contributor

Actually I was suggesting that in # 3 :)

I modified my previous comment to clarify on which 3 levels the user can set the icon option.

@MauriceG
Copy link
Contributor

Hi @uGoMobi !

With the code above, it will not be possible to disable the icon at LI-level.

With my first PR for this feature it will ;-)

Maurice

@jaspermdegroot
Copy link
Contributor

@MauriceG

Yes you are right. Just noticed that while testing my code. @jasondscott is working on this now.

@MauriceG
Copy link
Contributor

Oh cool, thanks.

Jason Scott added 3 commits September 18, 2012 10:43
ul

This provides the ability for the follow cases:

1. set the option as default for all listviews using the $.mobile object
2. set (or override the default) icon with data-icon on a listview or with
the option icon if that listview is created programmatically
3. override the above for a list item with data-icon on that specific list
item
Moved listview icon option to the correct place alphabetically in docs.
@jasondscott
Copy link
Contributor Author

@toddparker @uGoMobi Can I pull this into master?

@toddparker
Copy link
Contributor

I think I'd rather wait for a week since this is a new feature and we're at RC. As soon as 1.2 final is tagged, then yes. Looking forward to this addition.

@jaspermdegroot
Copy link
Contributor

@jasondscott

Nice!

+1 for merging after 1.2 is out. If we land this in 1.2.1 let's not forget to pick in the 1.2-stable branch :)

@ghost ghost assigned jasondscott Sep 19, 2012
jasondscott added a commit that referenced this pull request Oct 9, 2012
Listview: Added option for the default list icon
@jasondscott jasondscott merged commit 4a9427c into master Oct 9, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants