Skip to content

Loading…

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

Merged
merged 0 commits into from

4 participants

@jaspermdegroot
jQuery Foundation member

Issue: Buttons with <button> 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 <button> 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 <button> 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

@frequent

I'm actually increasing the size of buttons in toolbars - if they have iconpos="notext".

When doing my "low-dexterity-button-makeover", I try to match their padding to the height of the standard buttons, so they are visually aligned. Maybe something to consider adding to a general mini-button makeover.

@jaspermdegroot
jQuery Foundation member

hi @frequent !

I noticed issue #1998 which you opened, but unfortunately after I already created PR #4154.
When @Wilto is done reviewing that one I could look into notext buttons, because if I would propose more changes right now it is going to be messy I think. If you like you can do a pull request yourself of course :-)

@toddparker

@uGoMobi - This is awesome work, sorry we're lagging behind on reviewing these but we're trying to catch up. Keep it coming!

@frequent

hi @uGoMobi
i will try to put something together over the weekend and try myself and pull-requesting. haven't quite figured that out :-)

@jaspermdegroot
jQuery Foundation member

@toddparker Thanks and I totally understand it takes a lot of time to review all requests. I would be worried if things got pulled in right away ;-)

@frequent Good to hear you going to try to join the club of contributors, but don't feel obligated because I (as just a fellow JQM user/fanatic) suggested it. Anyway... good luck with it and you can always email me if you have questions!

@scottjehl

I'm not sure I'm following all the reasoning for these changes, so bear with me while I make assumptions... and please correct me where I'm off. :)

While I agree that there's an issue to fix here, I'm not sure we should disable auto-initialization of button and input elements to fix it. Am I right in thinking that's what this change does? Auto-init'ing button elements into button widgets (without requiring data-role=button is similar to auto-initingselect`, text inputs, and others - it's an expected behavior of the framework. @toddparker do you agree?

To ensure buttons within toolbars default to mini (while still allowing them to be overridden with data-mini=false, it seems like we could adjust the logic in the mini default value.

Changing line 27 of buttonMarkup.js to the following does the job, but it's ugly and not very readable. Perhaps you'd have time to tinker? I used a closure within the ternary to avoid calling jqmData() twice, but still, there's surely a better way.

mini:       options.mini       !== undefined ? options.mini       : (function( data ){ return data !== undefined ? data : !!el.closest( ".ui-bar" ).length; }( el.jqmData( "mini" ) ))

Thoughts? Beyond that change, are the CSS commits a separate issue or the same?

@jaspermdegroot
jQuery Foundation member

First of all; ignore commit fd05da4 I see I have been merging into the wrong branch. Will try to remove the commit later.

The auto-init'ing of button and input elements is taken care of by the button widget already (initSelector). Removing them here only prevents that it is done twice when you do set data-role="button" on a input or button element. As the test page shows button and input elements will still be enhanced as buttons without the need to set the data-role.

Maybe I am wrong but I don't think the change you propose would work.
options.mini !== undefined will result in true (the button widget has defined it as false) so the value of options.mini (false) will be used.

The CSS commit fc8738a is related but does affect all kind of buttons, not only button/input elements. The buttonMarkup allows to override default inline and mini style and add classes ui-btn-block and ui-fullsize for this. The rules for buttons with those classes in header/footer are missing in the CSS. That's what gets fixed by this commit.

@jaspermdegroot jaspermdegroot merged commit 26bccaa into jquery:master
@jaspermdegroot
jQuery Foundation member

@scottjehl

Sorry, but somehow all commits were removed here.

"uGoMobi merged commit 26bccaa into jquery:master from uGoMobi:issue_#3998" looked kinda scary (I shouldn't be able to merge into jquery:master), but turned out to be some kind of fake merge (0 commits with 0 additions and 0 deletions).

I had to open a new PR: #4363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 0 additions and 0 deletions.
Something went wrong with that request. Please try again.