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

New listview option icon - change or disable icons on listviews #4582

Closed
wants to merge 2 commits into from
Closed

Conversation

MauriceG
Copy link
Contributor

Instead of change or disable icons on every list item, this option enables it per listview.
The default icon arrow-r can be changed or disabled at all.
The ability to change or enable icon for individual item still remains.

Demo at: http://jsfiddle.net/MauriceG/swtKy/
Fullscreen: http://jsfiddle.net/MauriceG/swtKy/show/light/

@ghost ghost assigned jaspermdegroot Jun 22, 2012
@jaspermdegroot
Copy link
Contributor

Related feature request: #4570

@jaspermdegroot
Copy link
Contributor

hi @MauriceG

I think the default for listview option icon should be icon: "arrow-r" instead of icon: "".

If we make it an option for listview we have to offer the possibility to set it with a data-attribute on the listview, not only programmatically. That's not in your code, right?

The value for the option icon for the call to buttonMarkup is now a very long line with OR statements. I suggest to create a variable.

That variable should be a function that checks if data-icon is set on the listview item. If not it should fall back to the data-icon attribute of the parent listview. If that has not been set as well it falls back to the option which is "arrow-r" by default but can be configured programmatically.

BTW - When we make this change we should not forget to update the listview options page in the docs and the data-attribute reference.

@MauriceG
Copy link
Contributor Author

MauriceG commented Jul 7, 2012

Hi @uGoMobi
May you can take another look ...

May I missed something, but you can:

  • set icon per listview
  • disable icon per listview
  • leave listview as is and default icon arrow-r will be used
  • overwrite icon per item if listview icon is set or disabled
  • all works programmatically or by setting data-icon on listviiew

I've tried to make the change as tiny as possible and I think the icon condition is still easy to read and there is no need for another function.

If this will get in, I hopefully will not forget the docs ;-)

Maurice

@jaspermdegroot
Copy link
Contributor

hi @MauriceG

You are right, it does work! Most probably I am the one who missed something here.

Below you'll see the code that I had in mind. This is basically the same thing written in another way, but with one difference.

That is listIcon = $list.jqmData( "icon" ) || o.icon which is required in this construction to see if data-icon is set on the ul with data-role="listview". This is why I thought that part was missing in your PR.

To be honest, I still don't understand how this works in your code. I only see icon = item.jqmData("icon"); which checks if data-icon is set on the li.
Maybe you can explain it to me.

$.widget( "mobile.listview", $.mobile.widget, {

    options: {
        icon: "arrow-r",
    },
    refresh: function( create ) {

        var o = this.options,
            $list = this.element,
            listIcon = $list.jqmData( "icon" ) || o.icon,
            icon;
                    var itemIcon = item.jqmData( "icon" );

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

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

@MauriceG
Copy link
Contributor Author

MauriceG commented Jul 8, 2012

Hi @uGoMobi
I've added a new icon option for listviews.
If a listview now gets the data-icon attribute like <ul data-role="listview" data-icon="grid">..., _getCreateOptionsin jquery.mobile.widget.js will set the icon option to that icon name. There is no need to check again with listview.jqmData ( "icon" ).
On a listview-widget, listview.jqmData ( "icon" ) and listview.options.icon have the same value or I'm wrong here?

Maurice

@jaspermdegroot
Copy link
Contributor

@MauriceG

If I look at http://test.jqmobile.de/js/widgets/listviewSetIcon.js which is loaded by your test page, I see your code how it was after the first commit. It doesn't include the changes of the second commit. Am I right?

I had a look at _getCreateOptions and now (finally) understand how this works. Thanks!

When I test your code (after the second commit) I see it works, except for one thing. The data-icon set on the ul with data-role="listview doesn't override the configured default. I think we do need to check with listview.jqmData ( "icon" ) to make this possible.

We shouldn't use new option names if not necessary, because that is not user friendly. So listIcon should be icon and if we don't use "arrow-r" here as default is should be null instead of "".

@MauriceG
Copy link
Contributor Author

MauriceG commented Jul 8, 2012

Hi @uGoMobi

With the first commit everything works fine. Also the data-icon setting on listview to override the listview default icon.
To show this I've created the fiddle in the note of the first commit.

But I let inspire myself by your notes and code examples to

  • a) remove the long or condition
  • b) change the icon variable name to listIcon
    because I thought this is as desired.

After the second commit still everything works except setting the list item data-icon to false :-(
Testable in the second fiddle which loads another js file: listviewSetIconV2.js with the changes of the second commit.

Maurice

@jaspermdegroot
Copy link
Contributor

hi @MauriceG

The good news is that if you just change "listIcon" to "icon" everything will be fine again :)
If you push another commit for that (and the null) it is good to go.
I will ask @toddparker if he agrees with landing this.

Jasper

@MauriceG
Copy link
Contributor Author

MauriceG commented Jul 9, 2012

Hi @uGoMobi

What a fight :-)

Maurice

@MauriceG MauriceG closed this Jul 9, 2012
@MauriceG
Copy link
Contributor Author

MauriceG commented Jul 9, 2012

@uGoMobi
But I don't know, which version you mean now...
The second commit (contains icon = item.jqmData("icon") || o.icon;), it is not possible to disable the icon by data-icon="false".
And the first commit contains the long or condition you don't like ...
Maurice

@jaspermdegroot
Copy link
Contributor

hi @MauriceG

Why did you close? We are not done yet ;-)

I meant adding a 3rd commit so that would mean changing version 2. You are right though, data-icon="false" on the list item won't work then.

Your version one works fine. It was just that at the beginning it wasn't clear to me how, but now it is.

It's true though that I am not in favor of having an option defined by a short-hand if statement with multiple or statements wrapped inside it.

I suggest we ask @johnbender or @gseguin what they think of landing your first commit.

Jasper

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.

2 participants