Button Re-write #1333

arschmitz · 2014-09-02T15:22:10Z

This replaces PR #1216

This includes new widget controlgroup which replaces buttonset
Current known issues

Checkboxradio testfailure missing default element (@scott_gonzalez said it should not have one because it cant be sane ) this causes common tests to fail
Backcompat: Draft this on the wiki
Controlgroup icon only buttons 1px too short in chrome, firefox extra weird @jaspermdegroot think you could take a look?
demos/controlgroup/default.html As above, labels should demo something
demos/checkboxradio/default.html doesn't have the "command line" anymore, but still doesn't look like a demo. Labels should reflect some real world usecase.
demos/button/icons.html still looks like a visual test, not a demo

jzaefferer · 2014-09-02T18:25:05Z

demos/button/icons.html

 <div class="demo-description">
 <p>Some buttons with various combinations of text and icons.</p>
 </div>
+<div class="widget">
+	<h1>Widget</h1>
+	<a>Button with icon only</a>


This is missing an href attribute. Or it should be a <button> as well.

jaspermdegroot · 2014-09-03T10:33:00Z

If you remove the font-size: 62.5%; rule from demos.css you'll see there are some more issues with controlgroups;

There is space between the buttons in horizontal controlgroups
On Chrome you only get to see ellipsis in the dropdown buttons
There is almost no space between the text and the icon in the dropdown button (noticed on Firefox)

Screenshot Chrome 37 / MBP Retina:

Screenshot Firefox 32 / MBP Retina:

arschmitz · 2014-09-03T14:24:59Z

@jaspermdegroot this is fixed and also fixed the off by one i mentioned but in firefox icon only buttons are still a little short can you try and take a look at this?

jaspermdegroot · 2014-09-03T18:22:18Z

@arschmitz

I pulled the branch this morning and I don't see any commits after that. Where is it fixed?
Yes, I am looking into the icon-only buttons height problem on Firefox.

Update: @arschmitz - Nevermind, I don't know what went wrong but I forked the repo again and checked out the button branch and the problem has been fixed.

jaspermdegroot · 2014-09-04T09:25:08Z

themes/base/button.css

@@ -28,81 +29,79 @@
 }
 /* to make room for the icon, a width needs to be set here */


This comment should be removed

Update: We do need to set a width for anchor elements, so the comment should stay and we should revert the change where the width setting has been removed.

jaspermdegroot · 2014-09-04T11:27:26Z

PR #1340 contains the fix for the controlgroup icon-only button height problem on Firefox.

jaspermdegroot · 2014-09-08T09:08:12Z

I added 2 more commits to #1340 to fix the following issues:

When you try to toggle the state of a checkbox too fast you will select the text instead of changing the state. I added user-select: none; (not supported by IE8/9) for all buttons, not just checkbox buttons, to fix this.
On IE8 the radio icons were black which looked bad. I changed the opaque fallback background color to grey. Although the semi-transparent background color for browsers that do support rgba can be used across themes, it might be better to move this to theme CSS.
On IE8 the radio icon gets a black border in the active state because IE8 doesn't follow the specs (text color should have been used as border color if that property hasn't been set). Shouldn't we set the border-color to prevent this issue?

arschmitz · 2014-09-23T03:35:05Z

@jzaefferer this now includes the commits from @jaspermdegroot to fix controlgroup and various fixes / updates for demos.

jzaefferer · 2014-10-01T09:36:20Z

demos/button/default.html

@@ -11,7 +11,7 @@
 	<link rel="stylesheet" href="../demos.css">
 	<script>
 	$(function() {
-		$( "input[type=submit], a, button" )
+		$( ".widget input[type=submit], .widget a, .widget button" )
 			.button()
 			.click(function( event ) {
 				event.preventDefault();


Let's apply the preventDefault() to all buttons on this page, not just the widget buttons.

jzaefferer · 2014-10-01T09:39:42Z

One button unit test is currently failing.

jzaefferer · 2014-10-01T09:43:03Z

demos/checkboxradio/default.html

+	<h2>Checkbox</h2>
+
+	<fieldset>
+		<legend>Rating</legend>


This is an improvement and more demo-like. Though as-is a bit irritating: It looks like its a part of a rating UI, where it wouldn't make sense to select multiple ratings. Maybe change the legend to "Filter by rating"?

As for the markup, shouldn't they all share the same name and have a value attribute? Maybe use "rating" instead of "checkbox"?

jzaefferer · 2014-10-01T10:39:16Z

Can checkboxradio also support a "CSS only" mode?

jzaefferer · 2014-10-01T10:41:21Z

ui/checkboxradio.js

+		formElement.off( "reset" + this.eventNamespace, formResetHandler );
+		formElement.on( "reset" + this.eventNamespace, formResetHandler );
+
+		// If it is null the user set it explicitly to null so we need to check the dom


Let's uppercase "dom" to "DOM".

Conflicts: ui/checkboxradio.js

Cant be sane

And simplify css

This widget replaces the buttonset widget

Methods of new widgets added via the items option.

gabrielschulhof · 2014-11-10T08:00:21Z

ui/checkboxradio.js

+	},
+
+	_enhance: function() {
+		var checked = this.element.is( ":checked" );


Isn't it faster to use this.element.prop( "checked" )?

jaspermdegroot · 2014-11-22T11:23:44Z

@arschmitz

The negative margin for `ui-button`` in horizontal controlgroups still leaves space between some of the buttons. Which buttons depends on what browser you're on and what font size you use.
Another downside of this solution is that if there is no linebreak or space in the markup between the controlgroup elements it will cause overlap.
Have you looked into making the buttons float?

In case we stick to inline-block and negative margin:

Why negative margin left and right and not just right?
ui-button shouldn't get negative margin left and right inside vertical controlgroups

The width of the select button in a vertical controlgroup should be auto. Currently it will break when you increase the font size (not only because of the inline width style, also because of the negative margin)

gabrielschulhof · 2014-12-11T20:36:45Z

ui/button.js

-			// we don't search against the document in case the element
-			// is disconnected from the DOM
-			ancestor = this.element.parents().last();
-			labelSelector = "label[for='" + this.element.attr("id") + "']";


You need to selector-escape the ID, in case it contains funky characters which will render the selector invalid:

... + this.element.attr( "id" ).replace( /([!"#$%&'()*+,./:;<=>?@[\]^`{|}~])/g, "\\$1" ) + ...

I guess we should follow the SOP and put the regex into a variable to the top of the file, so we don't recompile it.

sfrisk · 2014-12-17T16:42:15Z

I agree with @jaspermdegroot regarding control group issues might be fixed by using floats instead of keeping them inline-blocks with the negative margin adjustment. As much as I like inline-blocks, if you're dealing with a case where elements need to be right up against each other, floats typically are the better solution. The negative margins css hack for inline-blocks can be tricky, and the alternative is making sure there are no spaces between your elements in the html, which tends to make the html look messy.

arschmitz · 2015-01-08T03:23:33Z

@jaspermdegroot Turns out its actually not possible to set width auto on selectmenu buttons in a vertical controlgroup which is why I was not. Selectmenu sets an actual width on the button via JS no matter what setting auto actually breaks this and makes it tiny. 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

arschmitz · 2015-01-08T04:02:03Z

Closing replaced by #1415

jaspermdegroot · 2015-01-08T13:30:09Z

@arschmitz

setting auto actually breaks this and makes it tiny

I don't see that problem. The screenshot below shows the result when I set width: auto important!; for the selectmenu button in the vertical controlgroup and margin: 0; for all buttons in the vertical controlgroup. To illustrate the problem I set font-size: 22px; on the body.

With current CSS, changing the font-size to 22px results in this:

Using !important to override the width that the JS sets on the button should be avoided, so I agree this should be resolved in the selectmenu widget so it doesn't set a width when the select is in a vertical controlgroup.

Ps. I think you forgot to actually close this PR :)

arschmitz · 2015-01-08T13:35:23Z

@jaspermdegroot exactly and using !important is not acceptable. It makes it essentially impossible for anyone to override. I'm surprised this is not in the style guide /cc @sfrisk

jaspermdegroot · 2015-01-08T14:22:01Z

Well, you can override it. Just need to use !important in your override as well and make sure it comes after the framework CSS. But, like I said, we should avoid using it and it might indeed be good to add a note about that to the style guide.

Will the new selectmenu widget still use JS to set a width as far as you know? I was wondering what problem had to be solved that couldn't be done with CSS only.

arschmitz · 2015-01-08T14:24:30Z

@jaspermdegroot I'm not sure what you mean by new selectmenu widget? This is the UI widget which just landed in 1.11 ( the current stable version )

jaspermdegroot · 2015-01-08T14:31:18Z

@arschmitz - Sorry, I had Mobile in my mind where we didn't adopt the new selectmenu yet.

jzaefferer reviewed Sep 2, 2014
View reviewed changes

arschmitz force-pushed the button branch from d41c02e to 2697371 Compare September 3, 2014 14:23

jaspermdegroot reviewed Sep 4, 2014
View reviewed changes

jzaefferer reviewed Oct 1, 2014
View reviewed changes

arschmitz added 9 commits November 7, 2014 23:40

Checkboxradio: allow the parent of the input to be the label

80cb6e7

Conflicts: ui/checkboxradio.js

Checkboxradio: Fixup demos

7ac96c8

Checkboxradio: Remove default element

4fde24d

Cant be sane

Checkboxradio: add classes option

2f4bf6a

Checkboxradio: Added new product selector demo

2fc590f

Checkboxradio: change all classes to be prefixed with ui-checkboxradio

c70e383

And simplify css

Controlgroup: Inital commit of new widget

987a83f

This widget replaces the buttonset widget

Controlgroup: add classes option

35697d4

Controlgroup: Add extension points for special options and refresh

3e1478a

Methods of new widgets added via the items option.

gabrielschulhof reviewed Nov 10, 2014
View reviewed changes

Button: breakup deprecated tests

5969ce6

arschmitz force-pushed the button branch from 93ed8ae to 5969ce6 Compare November 12, 2014 17:17

arschmitz added 2 commits December 3, 2014 11:55

Button: Add backcompat tests and fix backcompat issues

2d04b3e

Checkboxradio: formatting

8e28a60

gabrielschulhof reviewed Dec 11, 2014
View reviewed changes

arschmitz mentioned this pull request Jan 8, 2015

Button classes #1415

Closed

arschmitz closed this Jan 8, 2015

arschmitz deleted the button branch April 3, 2015 17:27

		@@ -28,81 +29,79 @@
		}
		/* to make room for the icon, a width needs to be set here */

Button Re-write #1333

Button Re-write #1333

Uh oh!

Conversation

arschmitz commented Sep 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaspermdegroot commented Sep 3, 2014

Uh oh!

arschmitz commented Sep 3, 2014

Uh oh!

jaspermdegroot commented Sep 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaspermdegroot commented Sep 4, 2014

Uh oh!

jaspermdegroot commented Sep 8, 2014

Uh oh!

arschmitz commented Sep 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Oct 1, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Oct 1, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaspermdegroot commented Nov 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfrisk commented Dec 17, 2014

Uh oh!

arschmitz commented Jan 8, 2015

Uh oh!

arschmitz commented Jan 8, 2015

Uh oh!

jaspermdegroot commented Jan 8, 2015

Uh oh!

arschmitz commented Jan 8, 2015

Uh oh!

jaspermdegroot commented Jan 8, 2015

Uh oh!

arschmitz commented Jan 8, 2015

Uh oh!

jaspermdegroot commented Jan 8, 2015

Uh oh!

Uh oh!