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

Various issues with Button #973

Open
3 of 5 tasks
drjayvee opened this issue Jul 8, 2013 · 12 comments
Open
3 of 5 tasks

Various issues with Button #973

drjayvee opened this issue Jul 8, 2013 · 12 comments
Labels

Comments

@drjayvee
Copy link

drjayvee commented Jul 8, 2013

First of all, let me know if I should split this into several bugs.

  1. Buttons don't preserve type attribute (which makes them submit forms by default). I have a pull request for that, but it's incomplete.
  2. The source has several hacks or otherwise weird design:
  3. The examples in Quick Start docs don't render() the Buttons. Without render(), bindUI() and syncUI() aren't called.
  4. Button supports only text labels. I'd like to use something like
    <button type="button"><img src="sprite.png"> Toggle</button>
    (which is valid HTML), but that's not currently possible. A plugin would do the job, but as noted above, ButtonPlugin only provides the ButtonCore API.
  5. ButtonGroup.disable() doesn't disable its children (I'll create a pull request for that)

I'd like to work on these issues, but I need some detailed guidance.


Complete

@rgrove
Copy link
Contributor

rgrove commented Jul 8, 2013

@drjayvee Thanks for tackling these. @derek wrote most of Button and ButtonGroup, so he should be able to offer guidance.

@ghost ghost assigned derek Jul 8, 2013
@drjayvee
Copy link
Author

drjayvee commented Jul 9, 2013

I've been thinking about the button type, and I think Buttons should really default to type="button" (instead of submit).

What about WidgetButton? Should we have them create type: "button" Buttons (by default)?

@drjayvee
Copy link
Author

Just wanted to let you know that I'll be on vacation until the end of the month.

@derek
Copy link
Contributor

derek commented Jul 26, 2013

Just a heads up that I plan to dig into this once I finish up some work on a current project. Hopefully next week.

@drjayvee
Copy link
Author

@derek I'm back from vacation, so let me know if you want any help!

@derek
Copy link
Contributor

derek commented Aug 23, 2013

Filed PR #1125 to improve support for single-box widgets. So now any existing node attributes (e.g. type="button", id="foobar") don't get blown away on render and the source node is preserved as-is.

Now it appears simply changing Y.ButtonCore.prototype.TEMPLATE (src) to TEMPLATE: '<button type="button"/>' will default all buttons to type=button (instead of submit), which may negate the need for an addition ATTR introduced in #968. Despite that being off-spec (HTML button spec), I think it's safe to assume the 80% use-case for Button widgets is for non-submit buttons.

Experimentation continues...

@drjayvee
Copy link
Author

PR #1125 is great for Button! Not only does this preserve the type attribute, but any additional classes, correct?

@derek
Copy link
Contributor

derek commented Aug 26, 2013

Yup. Classes, types, IDs, and any additional attributes and nested HTML are all preserved. The srcNode is no longer removed and replaced with the boundingBox, the srcNode is the boundingBox (as well as contentBox).

@derek
Copy link
Contributor

derek commented Sep 4, 2013

Submitted #1163 to address checklist item 4.

The Widget update (checklist item 1) was included as part of today's YUI 3.13.0 Beta 1 release.

@derek
Copy link
Contributor

derek commented Sep 5, 2013

#1163 has been updated to clean up Y.Button & Y.ButtonCore's architecture, which I believe also addresses checklist item 2.

To answer the specific issues:

ButtonCore has a _renderUI method, even though it doesn't extend Widget.

ButtonCore is intended to emulate parts Widget, but not force the weight of Widget into the lowest level of the Button component. Basically, the intent is when you combine Y.ButtonCore with Y.Widget, and fill in a few additional properties (which you can see here), Y.ButtonCore becomes a Widget.

Button's HTML_PARSER.label sets an instance variable (marked with TODO: remove)

Removed.

As noted in button.js, toggle functionality is currently not pluggable

Still a work in progress.

@drjayvee
Copy link
Author

drjayvee commented Sep 9, 2013

Alrighty, Button is getting some love. I like the progress so far.

@derek
Copy link
Contributor

derek commented Sep 10, 2013

Added a wiki to track ongoing Button-related work, https://github.com/yui/yui3/wiki/Button-wishlist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants