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

fix(material-experimental/mdc-slider): fix change events on slider in… #22286

Merged
merged 4 commits into from Mar 23, 2021

Conversation

wagnermaciel
Copy link
Contributor

…puts

  • create GlobalChangeListener to handle listening for change events that occur on the document
  • stop all of the slider inputs change events from reaching users
  • dispatch our own fake change events from #emitChangeEvent in the slider adapter
  • use the GlobalChangeListener for change events instead of adding our own event listener
    in #registerInputEventHandler
  • keep track of and unsubscribe from the GlobalChangeListener in
    #deregisterInputEventHandler

…puts

* create GlobalChangeListener to handle listening for change events that occur on the document
* stop all of the slider inputs change events from reaching users
* dispatch our own fake change events from #emitChangeEvent in the slider adapter
* use the GlobalChangeListener for change events instead of adding our own event listener
  in #registerInputEventHandler
* keep track of and unsubscribe from the GlobalChangeListener in
  #deregisterInputEventHandler
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 18, 2021
@wagnermaciel
Copy link
Contributor Author

I'm still working on the unit tests for this atm

const subject = this._subjects.get(type)!;
const handler = this._handlers.get(type)!;

return subject.pipe(finalize(() => {
Copy link
Member

@crisbeto crisbeto Mar 22, 2021

Choose a reason for hiding this comment

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

Returning a subscription means that we can't use operators on the same stream. You can check out src\cdk\scrolling\scroll-dispatcher.ts for an example of how this can be done using a custom observable.

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 think for this case, returning a subscription is sufficient. I think if we reach a point where we need to start using operators on the stream it would make sense to extend the functionality this way, but for the current use case and for the foreseeable future, this should be fine

@wagnermaciel
Copy link
Contributor Author

@crisbeto @mmalerba

I finished trying out using the native inputs under the hood and concluded that while it is possible, the cons outweigh the pros. The main issue with using native inputs is that for range sliders is this:

In the case where the two thumbs are on top of each other, there is no intuitive way to decide which slider thumb the user is interacting with. Because the MDC implementation is not using the native inputs, it can wait until the user begins dragging in a direction to decide which thumb should be focused/active. With native sliders, you must decide which thumb is focused before the user even triggers a mousedown.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@wagnermaciel wagnermaciel merged commit 5b34bd1 into angular:mdc-slider Mar 23, 2021
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Mar 23, 2021
angular#22286)

* fix(material-experimental/mdc-slider): fix change events on slider inputs

* create GlobalChangeAndInputListener to handle listening for change events that occur on the document
* stop all of the slider inputs change events from reaching users
* dispatch our own fake change events from #emitChangeEvent in the slider adapter
* use the GlobalChangeAndInputListener for change events instead of adding our own event listener
  in #registerInputEventHandler
* keep track of and unsubscribe from the GlobalChangeAndInputListener in
  #deregisterInputEventHandler
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Apr 1, 2021
angular#22286)

* fix(material-experimental/mdc-slider): fix change events on slider inputs

* create GlobalChangeAndInputListener to handle listening for change events that occur on the document
* stop all of the slider inputs change events from reaching users
* dispatch our own fake change events from #emitChangeEvent in the slider adapter
* use the GlobalChangeAndInputListener for change events instead of adding our own event listener
  in #registerInputEventHandler
* keep track of and unsubscribe from the GlobalChangeAndInputListener in
  #deregisterInputEventHandler
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Apr 2, 2021
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Apr 2, 2021
angular#22286)

* fix(material-experimental/mdc-slider): fix change events on slider inputs

* create GlobalChangeAndInputListener to handle listening for change events that occur on the document
* stop all of the slider inputs change events from reaching users
* dispatch our own fake change events from #emitChangeEvent in the slider adapter
* use the GlobalChangeAndInputListener for change events instead of adding our own event listener
  in #registerInputEventHandler
* keep track of and unsubscribe from the GlobalChangeAndInputListener in
  #deregisterInputEventHandler
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Apr 8, 2021
angular#22286)

* fix(material-experimental/mdc-slider): fix change events on slider inputs

* create GlobalChangeAndInputListener to handle listening for change events that occur on the document
* stop all of the slider inputs change events from reaching users
* dispatch our own fake change events from #emitChangeEvent in the slider adapter
* use the GlobalChangeAndInputListener for change events instead of adding our own event listener
  in #registerInputEventHandler
* keep track of and unsubscribe from the GlobalChangeAndInputListener in
  #deregisterInputEventHandler
@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 Apr 23, 2021
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.

None yet

2 participants