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

Selectmenu: Add documentation for _renderButtonItem extension point #226

Closed
wants to merge 1 commit into from

Conversation

fnagel
Copy link
Member

@fnagel fnagel commented Sep 3, 2014

No description provided.

var buttonItem = $( "<span>", {
"class": "ui-selectmenu-text"
})
this._setText( buttonItem, item.label );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is calling _setText() a requirement of a _renderButtonItem() extension? If so it would be good to mention in the <desc>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It set's the text on the element but it's not required. One could do:

_renderButtonItem = function( item ) {
    return $( "<span>", {
        "class": "ui-selectmenu-text",
        text: item.label
    });
};

@tjvantoll
Copy link
Member

The indentation in this file is off. Other than that and my other minor comment this looks good. We can land this with #223 whenever the code itself is merged in.

@fnagel
Copy link
Member Author

fnagel commented Sep 4, 2014

Fixed the indentation.

.css( "padding-left", "2.5em" )
.append( "<span>" )
.find( "span" )
.addClass( "ui-icon " + item.element.attr( "data-icon") )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent after .find().

@scottgonzalez
Copy link
Member

Changed the example and landed in 234c811 (created 1-12 branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants