Skip to content

Button classes #1415

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

Closed
wants to merge 107 commits into from
Closed

Button classes #1415

wants to merge 107 commits into from

Conversation

arschmitz
Copy link
Member

Replaced by gh-1513


Updates the previous button pr #1333 to the new _add/_remove/_toggleClass api

Of note is a significant re-thinking and simplification of the logic in controlgroup.

@arschmitz arschmitz mentioned this pull request Jan 8, 2015
6 tasks
}
}
return radios;
// We find .ui-button first then filer by :ui-button because doing a
Copy link
Member

Choose a reason for hiding this comment

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

Typo, "filter" should be "filter". Also drop the "a" at the end.

@jzaefferer
Copy link
Member

Leftovers from #1333:

  • demos/controlgroup/splitbutton.html still doesn't use the select option. Add a select event handler that also alerts something.
  • Can checkboxradio also support a "CSS only" mode?
  • radioGroup definition, commented there

There are some more comments, most of them resolved, except for the discussion about selectmenu width:

It seems like there should be a way to completely stop the selectmenu from setting a width for cases like this where you want it to fill a containers width. /cc @fnagel


<div class="demo-description">
<p>Examples of the markup that can be used for buttons: A button element, an input of type submit and an anchor.</p>
<p>Buttons can be styled via the button widget for by adding the classes yourself to avoid the javascript overhead if you dont need any of the methods provided by the button widget</p>p>
Copy link
Member

Choose a reason for hiding this comment

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

Bad markup, delete the p> at the end of the line

@arschmitz
Copy link
Member Author

@jzaefferer

demos/controlgroup/splitbutton.html still doesn't use the select option. Add a select event handler that also alerts something.

Yes it does that was fixed in #1333 and here see https://github.com/jquery/jquery-ui/pull/1333/files#r22649864

Can checkboxradio also support a "CSS only" mode?

Not with our browser support its not possible

radioGroup definition, commented there

Pushed commit already arschmitz@9afd80b

</button>
<button class="ui-button ui-widget ui-corner-all ui-icon-bottom">
<span class="ui-icon ui-icon-triangle-1-s"></span>
Button with icon on bottom
Copy link
Member

Choose a reason for hiding this comment

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

"Button with icon on the bottom", to make it consistent with the text above

@jzaefferer
Copy link
Member

Yes it does that was fixed in #1333 and here see https://github.com/jquery/jquery-ui/pull/1333/files#r22649864

Okay, I misread the code. Using "change" seems fine, but why the separate handler instead of using the change option?

Also replacing the alert with appending some text to the page would be good.

<legend>Filter by location: </legend>
<label for="location-new-york">New York</label>
<input type="radio" name="location" id="location-new-york">
<label>Paris
Copy link
Member

Choose a reason for hiding this comment

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

Why is this mixing "input inside label" with other variants? Doesn't belong in a demo.

@arschmitz
Copy link
Member Author

@jzaefferer regarding the layout issues in the demos looks like this happened when i rebased with master and the demo font size changed ill go through them all and check the layouts

}
}).selectmenu( "widget" ).removeClass( "ui-icon-end" );
$( ".controlgroup" ).controlgroup();
$( "select" ).on( "selectmenuchange", function(){
Copy link
Member

Choose a reason for hiding this comment

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

Should convert this to the change option as part of the .selectmenu() call.

@jzaefferer jzaefferer added this to the land-before-esformatter milestone Mar 13, 2015
@arschmitz arschmitz mentioned this pull request Mar 19, 2015
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.

5 participants