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

Selection buttons #121

Merged
merged 13 commits into from
Jul 30, 2014
Merged

Selection buttons #121

merged 13 commits into from
Jul 30, 2014

Conversation

tombye
Copy link
Contributor

@tombye tombye commented Jul 25, 2014

Script to support a specific design of radio buttons and checkboxes wrapped in <label> tags:

<label>
  <input type="radio" name="size" value="medium" />
</label>

The script allows the label styling to reflect the state of the input inside because changes in state cause classes to be added or removed from the parent label. This makes the following styling possible:

Normal state:

image

Focused state:

image

Focused and selected state:

image

A test page is available in the Gist to see the script working in-place. It requires your placing the selection-buttons.js script in the same folder as the page.

https://gist.github.com/tombye/4875fb3ace4f67dfe654

@dsingleton
Copy link
Contributor

Would it be worth including checkboxes in this?

They have the same behaviour in elements, and we'll need to add that at some point. It might be worth doing now, rather than having to shoe-horn in later.

@edds
Copy link
Contributor

edds commented Jul 25, 2014

Can you add some readme or docs for this. I am not entirely sure what it does.

@tombye
Copy link
Contributor Author

tombye commented Jul 25, 2014

@dsingleton @edds thanks, good points all. Will include checkboxes and actually explain the thing.

@tombye tombye closed this Jul 25, 2014
@tombye tombye reopened this Jul 28, 2014
@tombye tombye changed the title Radio buttons Selection buttons Jul 28, 2014
_this[optionName] = opts[optionName];
});
}
this.setup();
Copy link
Contributor

Choose a reason for hiding this comment

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

setup doesn't feel like a very good name. What does this method actually do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to split any code that sets up the instance object into:

  1. any commands related to the arguments sent into the constructor (which is in the constructor)
  2. anything else in a separate method.

Explained that way I suppose it is ambiguous because it represents a container for every other action that needs to be performed around adding properties/methods to the instance.

In this context it's only really doing one thing, querying the DOM to find the radio in the set that's selected so maybe getSelection?

Copy link
Contributor

Choose a reason for hiding this comment

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

The separation is a good thing. Methods named generic things like setup often get used as a dumping ground so are best avoided.

I would possibly go with getSelections because it does it on every element. But getSelection would be perfectly agreeable.

edds added a commit that referenced this pull request Jul 30, 2014
@edds edds merged commit ecb935a into master Jul 30, 2014
@edds edds deleted the radio-buttons branch July 30, 2014 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants