Fixes #3998 - All buttons, regardless of markup, should be mini by default in toolbars #4363

Merged
merged 6 commits into from May 21, 2012

Conversation

Projects
None yet
2 participants
Member

jaspermdegroot commented May 15, 2012

This is a copy of PR #4291 which already has a thread but commits were lost

Issue: Buttons with as markup do not default to mini in header and footer.
Cause: The button widget sets mini by default to false. The buttonMarkup function provides in an option to control the styling of buttons in headers/footer; when mini is set to false ui-fullsize class is added and the default mini style won't apply.
Fix: Removed default from button widget in forms.button.js

Additional 1:
The same goes for inline style. When set to false the class ui-btn-block is added and the default inline style won't apply.
Issues: There are no rules for .ui-header/footer .ui-btn-block in the CSS so this doesn't work at all. The button widget also sets inline by default to false.
Fix: Added ui-btn-block rules to button.css and removed default from button widget in form.button.js

Additional 2:
Issue: When data-role="button" is added to a

element the buttonMarkup function will be initialized by the element itself as well as the button widget which results in a button wrapped in another button.
Fix: Excluded element from auto initialization in buttonMarkup.js

[Update:] Problem as mentioned at Additional 2 also applies to input elements. I pushed another commit to exclude those as well. This fixes: #3005

Passed all unit tests. Live test page: http://ugomobi.github.com/toolbar-button/

This PR fixes #3998

Contributor

scottjehl commented May 16, 2012

Thanks again, @ugomobi!

Overall, I think this seems to do the trick, but it seems to come at the expense of removing any way to programmatically set the mini and inline options that currently exist in the button plugin. A change like that would require a point release, I think, but @johnbender can chime in. Is there no way that you can see to make this change without changing the button plugin api?

Contributor

scottjehl commented May 16, 2012

...in other words, there are three ways to set these options: via data attributes, which you've manually covered here, via option hash when manually calling a plugin method, and via the plugin's prototype. I'm pretty sure this change prevents the latter 2 ways from working.

Member

jaspermdegroot commented May 16, 2012

@scottjehl hi Scott,

Thanks for your comments.
Now you have said it, I think you are right about not being able anymore to programmatically set those options. I have to look into it again.
Do you agree we can remove input and button elements from buttonMarkup auto init after reading my comment at the old PR?

Jasper

Contributor

scottjehl commented May 16, 2012

Sorry, yes I do agree on that part. Please continue on with this, and sorry it's so tricky!

On May 16, 2012, at 11:50 PM, Jasper de Groot wrote:

@scottjehl hi Scott,

Thanks for your comments.
Now you have said it, I think you are right about not being able anymore to programmatically set those options. I have to look into it again.
Do you agree we can remove input and button elements from buttonMarkup auto init after reading my comment at the old PR?

Jasper


Reply to this email directly or view it on GitHub:
#4363 (comment)

Member

jaspermdegroot commented May 17, 2012

@scottjehl

I don't see any possibility to make this happen without touching the button widget, but I did manage to make it work and still keep the possibility to set the options programmatically. I tested it and it works :-)

Contributor

scottjehl commented May 21, 2012

This is looking pretty darn great to my eye. Fine work, @ugomobi. Merging now...

scottjehl pushed a commit that referenced this pull request May 21, 2012

Merge pull request #4363 from uGoMobi/issue_#3998
Fixes #3998 - All buttons, regardless of markup, should be mini by default in toolbars

@scottjehl scottjehl merged commit db28688 into jquery:master May 21, 2012

Member

jaspermdegroot commented May 21, 2012

Thanks Scott. I will commit a test for this after I figured out how to write them. Jasper

Contributor

scottjehl commented May 21, 2012

Cool. Fwiw, these slides are a great start. http://benalman.com/talks/unit-testing-qunit.html

On May 21, 2012, at 11:21 AM, Jasper de Groot wrote:

Thanks Scott. I will commit a test for this after I figured out how to write them. Jasper


Reply to this email directly or view it on GitHub:
#4363 (comment)

jaspermdegroot added a commit that referenced this pull request Jul 12, 2012

Merge pull request #4363 from uGoMobi/issue_#3998
Fixes #3998 - All buttons, regardless of markup, should be mini by default in toolbars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment