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

Button icon span #1216

Closed
wants to merge 37 commits into from
Closed

Button icon span #1216

wants to merge 37 commits into from

Conversation

arschmitz
Copy link
Member

Working branch right now up for functional review for button re-write demo at http://view.jqueryui.com/button-icon-span/demos/button/

Spec: http://wiki.jqueryui.com/w/page/71756381/Button

@jzaefferer
Copy link
Member

As discussed at the meeting today, using space key on anchors should be supported, currently it doesn't work.

@jzaefferer
Copy link
Member

There's is code in place checking for SPACE on a elements, so something must be breaking that.

@jzaefferer
Copy link
Member

Can you rebase this branch? This still has old file names and no UMD wrappers.

Move to using element stats rather then js class states remove
ui-button-text spans.
Removed button set
},

_create: function() {
this.element.closest( "form" )
.unbind( "reset" + this.eventNamespace )
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if we can switch these to use _off/_on? I think at the time this was written, one or both didn't exist.

return options;
},

_isDisabled: function( options ) {
Copy link
Member

Choose a reason for hiding this comment

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

This is also ill-named in button. Methods starting with is generally indicated a boolean getter, but this actually reads from the DOM, then sets an option. _readDisabled should do. That would be consistent with _readType, too.

@jzaefferer
Copy link
Member

I'm done for now. I hope that someone else can look at the CSS changes, including the discussion we had about those (or just ask Alex). Maybe @tjvantoll?

@tjvantoll
Copy link
Member

I'd be happy to take a look at the CSS. Could you link me to the “discussion”? There's a lot here :)

@jzaefferer
Copy link
Member

It comes down to this: For CSS-only buttons to work, Alex moved a good
amount of button related CSS to core and theme CSS files. This looked
questionable to me and I didn't quite follow his argument.

Take a look at the diff for the CSS files. If it looks good to you, nothing
to worry about. Otherwise I'm sure Alex can explain again.

@arschmitz
Copy link
Member Author

@tjvantoll feel free to ping me for an explanation of any of the css.

border: none;
background-color: rgb( 0, 0, 0 );
background-color: rgba( 0, 0, 0, .3 );
opacity: .3;
Copy link
Member

Choose a reason for hiding this comment

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

We still support IE8 (which doesn't support rbga) so you probably need to include its proprietary thing here.

@tjvantoll
Copy link
Member

I get why you're putting the button styles in theme.css, and I think that's something we'll have to live with, at least until we use a CSS preprocessor and can use mixins. I'll do a little more thinking on this but I don't see a way around it.

I am confused why you have a bunch of button styles in core.css, as @jzaefferer alluded to here.

@arschmitz
Copy link
Member Author

Just talked about core.css with @tjvantoll and we both agree that anything for icons should remain in core.css assuming they will eventually be unscoped from .ui-button ( some things here and in theme are currently scoped only to avoid conflicts that will eventually be removed ) if they will remain scoped to .ui-button in the future they should be in button.css

additionally to make clear here other widgets will need to depend on button.css like checkboxradio, and selectmenu. This is a place mix-ins will help in the future.

@arschmitz arschmitz mentioned this pull request Sep 2, 2014
4 tasks
@arschmitz
Copy link
Member Author

closed replaced by #1332

@arschmitz arschmitz closed this Sep 2, 2014
@arschmitz arschmitz mentioned this pull request Sep 2, 2014
6 tasks
@arschmitz arschmitz deleted the button-icon-span branch January 8, 2015 03:56
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

4 participants