Skip to content

feat(slide-toggle): make slide-toggle click and drag actions configurable #11719

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 20, 2018
Merged

feat(slide-toggle): make slide-toggle click and drag actions configurable #11719

merged 1 commit into from
Jun 20, 2018

Conversation

tinayuangao
Copy link
Contributor

No description provided.

@tinayuangao tinayuangao requested a review from devversion as a code owner June 8, 2018 17:57
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 8, 2018
@tinayuangao tinayuangao requested a review from jelbourn June 8, 2018 17:57
@tinayuangao tinayuangao added the target: patch This PR is targeted for the next patch release label Jun 8, 2018
jelbourn
jelbourn reviewed Jun 8, 2018
View reviewed changes
src/lib/slide-toggle/slide-toggle-config.ts Outdated
/**
* Injection token that can be used to specify the slide toggle click behavior.
*/
export const MAT_SLIDE_TOGGLE_CLICK_ACTION =
Copy link
Member

Choose a reason for hiding this comment

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

Could you combine these two into one token MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS? We use that pattern on a handful of other components so it would be a bit more consistent (see MAT_AUTOCOMPLETE_DEFAULT_OPTIONS for an example)

devversion
devversion reviewed Jun 8, 2018
View reviewed changes
src/lib/slide-toggle/slide-toggle-config.ts Outdated
* check: Only toggle checked status. Default behavior
* undefined: Same as `check`.
*/
export type MatSlideToggleDragAction = 'noop' | 'check' | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

If someone sets this action to noop, wouldn't it make sense to just block dragging at all?

Also, does it make sense to call the other value check? Wouldn't toggle fit better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case is when user toggle/drag the slide toggle, a dialog popup and requires user's confirmation. After the user confirmed the value is changed.

I think we still need to enable dragging.

Changed to toggle

@tinayuangao tinayuangao force-pushed the slide-toggle-config branch from 6999690 to e6acc40 Compare June 11, 2018 17:57
@tinayuangao
Copy link
Contributor Author

Updated MatSlideToggleDefaultOptions. Please take a look. Thanks!

devversion
devversion approved these changes Jun 11, 2018
View reviewed changes
Copy link
Member

@devversion devversion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some small nits. I think it's really necessary to make clear what those two events do.

Shouldn't be mixed up with the change event.

Also: Asked a few questions on Slack about the use-case

src/lib/slide-toggle/slide-toggle.ts
@@ -195,10 +206,13 @@ export class MatSlideToggle extends _MatSlideToggleMixinBase implements OnDestro
// emit its event object to the component's `change` output.
event.stopPropagation();

if (!this._dragging) {
this.toggleChange.emit();
}
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 just move this.toggleChange.emit() down below the this._emitChangeEvent()? That's basically the same and removes the unnecessary check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This this.toggleChange.emit() also fires when this.defaults.disableToggleValue is true. When the value is true, the native element's checked status is reverted, and no value change is emitted.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

src/lib/slide-toggle/slide-toggle.ts Outdated
@@ -153,6 +157,12 @@ export class MatSlideToggle extends _MatSlideToggleMixinBase implements OnDestro
@Output() readonly change: EventEmitter<MatSlideToggleChange> =
new EventEmitter<MatSlideToggleChange>();

/** An event will be dispatched each time the slide-toggle input is toggled. */
@Output() readonly toggleChange: EventEmitter<void> = new EventEmitter<void>();
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit: I think it would be really appropriate to expand this comment further, to mention that this always fires, but does not mean that the slide-toggle value is always being updated. Same for the dragChange event.

Otherwise, I think there is the risk that people might use toggleChange over the usual change event.

src/lib/slide-toggle/slide-toggle.ts Outdated
// Releasing the pointer over the `<label>` element while dragging triggers another
// click event on the `<label>` element. This means that the checked state of the underlying
// input changed unintentionally and needs to be changed back.
if (this._dragging) {
if (this._dragging || this.defaults.disableToggleValue) {
Copy link
Member

Choose a reason for hiding this comment

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

Please update the comment to mention that we also revert the value back, if disableToggleValue is set to true.

@tinayuangao tinayuangao force-pushed the slide-toggle-config branch from e6acc40 to 94525da Compare June 11, 2018 21:43
jelbourn
jelbourn reviewed Jun 11, 2018
View reviewed changes
src/lib/slide-toggle/slide-toggle-config.ts Outdated
export const MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS =
new InjectionToken<MatSlideToggleDefaultOptions>('mat-slide-toggle-default-options', {
providedIn: 'root',
factory: MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS_FACTORY,
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to use an inline function here:

factory: () => ({disableToggleValue: false, disableDragValue: false}),

src/lib/slide-toggle/slide-toggle.spec.ts Outdated
provide: HAMMER_GESTURE_CONFIG,
useFactory: () => gestureConfig = new TestGestureConfig()
},
{ provide: MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS, useValue: {disableToggleValue: true}},
Copy link
Member

Choose a reason for hiding this comment

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

Extra space (here and below)

Copy link
Member

@devversion devversion Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should be also able to remove that MutationObserver thing. It's not really needed

@tinayuangao tinayuangao force-pushed the slide-toggle-config branch from 94525da to 9942a6e Compare June 12, 2018 17:49
@tinayuangao
Copy link
Contributor Author

Updated and fixed test. Please take a look. Thanks!

jelbourn
jelbourn approved these changes Jun 14, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jun 14, 2018
@tinayuangao tinayuangao merged commit 32193e9 into angular:master Jun 20, 2018
victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018
@tinayuangao @victoriaaa234
feat(slide-toggle): make slide-toggle click and drag actions configur…
961fe06 
…able (#11719)
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion devversion approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tinayuangao @jelbourn @devversion @googlebot