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

feat(): add slide-toggle component. #362

Merged
merged 1 commit into from
May 18, 2016

Conversation

devversion
Copy link
Member

@devversion devversion commented Apr 26, 2016

First step; Basic Skeleton for the Slide Toggle Component.

  • Readme
  • Visually Hidden Checkbox
  • Doesn't expose the EventEmitter
  • Replace Hue 400, 500 in background palette (@jelbourn)

Adressed in other PR's

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 26, 2016
@devversion devversion force-pushed the feat/slide-toggle-component branch 2 times, most recently from a7ae61c to 16c1cf9 Compare April 27, 2016 17:25
@jelbourn
Copy link
Member

jelbourn commented May 4, 2016

@devversion Now needs rebasing on my mega change #384. Feel free to ping me if you run into any issues.

@devversion devversion force-pushed the feat/slide-toggle-component branch 2 times, most recently from de7f505 to 1950cd5 Compare May 4, 2016 18:42
@devversion
Copy link
Member Author

@jelbourn Done :)

<div class="md-container">
<div class="md-bar"></div>
<div class="md-thumb-container">
<div class="md-thumb"></div>
Copy link
Member

Choose a reason for hiding this comment

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

For the slide-toggle, we want to drive the component with an underlying native checkbox, similar to what @robertmesserle is doing in #415 (since the slide-toggle is pretty much just a checkbox with a different appearance).

This gives you the accessibility and keyboard interaction for free.

@devversion devversion force-pushed the feat/slide-toggle-component branch 2 times, most recently from c7c45d9 to 92a56cf Compare May 12, 2016 20:16
## Theming
A slide-toggle is default using the `accent` palette for its styling.

Modifiying the color on a `slide-toggle` can be easily done, by using the following classes.
Copy link
Member

Choose a reason for hiding this comment

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

Need to update the readme for color="..."

@devversion devversion force-pushed the feat/slide-toggle-component branch 4 times, most recently from 9f8d20c to 32d7aac Compare May 16, 2016 13:25
[tabIndex]="tabIndex"
[checked]="checked"
[disabled]="disabled"
[attr.name]="name"
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to just do [name] without the attr.

Copy link
Member Author

@devversion devversion May 17, 2016

Choose a reason for hiding this comment

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

This is a good point. I also had that before. But this will cause issues with a falsy value.

For example, if the user, uses [name]="myName", and themyNameproperty is set tonull` will result in

<input name="null">

If you want to confirm it yourself. You can try setting $0.name = null in the Dev Tools on an input.

The advantage of [attr.name] is, that it removes the attribute, when it's falsy.

Same as in #447

Copy link
Member

Choose a reason for hiding this comment

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

After trying it out, you're right, attr.name is what we want for now.

if (this.checked !== !!value) {
this._checked = value;
this.onChange(this._checked);
this._change.emit(this._checked);
Copy link
Member

@jelbourn jelbourn May 17, 2016

Choose a reason for hiding this comment

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

One of the things I ran into recently was that checkbox and radio were firing change events for their initial value, which is unexpected. I fixed it by adding an _isInitialized field and setting it to true in ngAfterContentInit and then gating the emit on _isInitialized being true. This also prevents ng-pristine from being immediately changed to ng-dirty on first load. (see my PR #443).

Copy link
Member Author

@devversion devversion May 17, 2016

Choose a reason for hiding this comment

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

I tested that, and covered all possible situations by tests.

The issue, with firing events for the initial seems to be a more specific problem with the
I cannot reproduce this issue with the slide-toggle component.

I also noticed that as well some days ago - But it seems like I fixed it anyway.

I guess it's covered by this check in the setter.
Once an initial value get's emitted, then we compare the old value with the new one.
Which will be false in those cases.

    if (this.checked !== !!value) {

@jelbourn
Copy link
Member

Fantastic PR! Just a few comments for things that are in-flight for existing components.

@devversion devversion force-pushed the feat/slide-toggle-component branch 2 times, most recently from a26be92 to 218860e Compare May 17, 2016 15:40
`MdSlideToggle` is a two-state control, which can be also called `switch`

### Screenshots
![image](https://raw.githubusercontent.com/angular/code.material.angularjs.org/master/material2_assets/slide-toggle/toggles.png)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha 😄 I was already using that, but over the GitHub user content

@jelbourn
Copy link
Member

LGTM, just needs a rebase and to update the readme asset.

@devversion devversion force-pushed the feat/slide-toggle-component branch from 218860e to eee0766 Compare May 18, 2016 15:41
@devversion
Copy link
Member Author

@jelbourn Everything done

@jelbourn jelbourn merged commit e09a5bf into angular:master May 18, 2016
@devversion devversion deleted the feat/slide-toggle-component branch May 18, 2016 16:07
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants