feat(material-experimental/mdc-slider): implement the SliderAdapter #21844

wagnermaciel · 2021-02-08T23:09:04Z

complete the core logic for MatSliderThumb and MatSlider
collapse slider-thumb.ts and slider-adapter.ts into slider.ts

* complete the core logic for MatSliderThumb and MatSlider

wagnermaciel · 2021-02-08T23:21:33Z

@mmalerba & @jelbourn This is ready for review but I'm not sure how to neatly resolve this circular dependency. Should I just move the contents of slider-thumb.ts to slider.ts?

mmalerba · 2021-02-08T23:31:00Z

@jelbourn are we allowed to use type-only imports? https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export that would probably help with the circular deps

src/material-experimental/mdc-slider/slider-adapter.ts

* delete commented out code * add comment explaining why emitChangeEvent and emitInputEvent are ignored

src/material-experimental/mdc-slider/slider-adapter.ts

src/material-experimental/mdc-slider/slider.ts

src/material-experimental/mdc-slider/slider-adapter.ts

src/material-experimental/mdc-slider/slider-thumb.ts

src/material-experimental/mdc-slider/slider.ts

@inputs

* remove circular dependency between MatSlider and MatSliderThumb * change the selectors for MatSliderThumb * add a comment describing why and how the value property on the MatSliderThumb works * delete min, max, and step @inputs from MatSliderThumb * remove _initialize property from MatSliderThumb * move all of the MatSliderThumb initialization logic into ngAfterViewInit * remove input initialization logic from MatSlider ngAfterViewInit * add input validation logic to MatSlider ngAfterViewInit * fix value indicator text bug where the DOM was not getting updated

* add todo to remember to add the mdc-slider back into the kitchen sink ssr app * move _foundation.init() into the _platform.isBrowser check to avoid ssr problems

src/material-experimental/mdc-slider/slider-adapter.ts

src/material-experimental/mdc-slider/slider-thumb.ts

src/material-experimental/mdc-slider/slider-adapter.ts

crisbeto · 2021-02-12T16:13:07Z

src/material-experimental/mdc-slider/slider-adapter.ts

  }
-  emitDragEndEvent(value: number, thumb: Thumb): void {
-    throw Error('Method not implemented.');
+  getValueToAriaValueTextFn = (): ((value: number) => string) | null => {


Did we ever figure out why these have to be arrow functions instead of methods?

Yes, MDC is clobbering them. I left a comment here explaining
https://github.com/angular/components/pull/21844/files#diff-f0898cf25f96d9c72cf31d7289a09209c713131e3268f7c96c27b35cc4be537bR39

src/material-experimental/mdc-slider/slider-thumb.ts

src/material-experimental/mdc-slider/slider.ts

* make input validation only happen in ngDevMode * move _validateInputs() and _throwInvalidInputConfigurationError() out of MatSlider so they can be tree-shaken away in production mode * delete _getValueIndicatorText() and _getValueIndicatorTextElement() and add their logic to _setValueIndicatorText() * made getters more efficient * deleted unused _getValue() method * move export class MatSliderThumb ... to its own separate line * moved MatSliderThumb value initialization to the constructor to avoid race conditions when setting the min and max * set the value property in ngAfterViewInit for MatSliderThumb

src/material-experimental/mdc-slider/slider-adapter.ts

src/material-experimental/mdc-slider/slider-thumb.ts

* move dummy MatSlider interfaces from slider-adapter and slider-thumb to slider-interface * create MatSliderThumb and MatSliderDragEvent dummy interfaces to avoid a circular dep between slider-thumb and slider-interface when defining the dummy MatSliderThumb interface * make MatSlider implement the dummy _MatSliderInterface

* add class description jsdoc * move comment explaining why we use arrow fns

…r change detection problem * use markForCheck() instead of detectChanges() in updateTickMarks() * call detectChanges() once at the end of ngAfterViewInit() in MatSlider with a thorough comment explaining why it is needed

…r.ts * delete slider-adapter.ts * delete slider-interface.ts * delete slider-thumb.ts * move SliderAdapter implementation to slider.ts * move MatSliderThumb implementation to slider.ts * go back to using direct references instead of interfaces * fix imports in module.ts * fix imports in public-api.ts

mmalerba

LGTM with a couple API nits

src/material-experimental/mdc-slider/slider.ts

* delete empty providers * rename thumb to _thumb and make it private in MatSliderThumb * delete thumb property in MatSliderDragEvent

src/material-experimental/mdc-slider/slider.scss

src/material-experimental/mdc-slider/slider.ts

jelbourn · 2021-02-16T23:54:17Z

src/material-experimental/mdc-slider/slider.ts

+  @Output() readonly _focus: EventEmitter<void> = new EventEmitter<void>();
+
+  /** Indicates which slider thumb this input corresponds to. */
+  private _thumb: Thumb;


I would call this _thumbPosition and the enum ThumbPosition

The Thumb enum comes from MDC

Ack, I would still change the name of the property

Ok. Would you also like me to make this change to all of the SliderAdapter and MatSlider methods that use thumb as the name for this?

Yeah, any properties/variables that refer to this concept I would name thumbPosition since it's a much more clear description than thumb (which I would assume is a reference to the actual thumb itself, not just the position)

src/material-experimental/mdc-slider/slider.ts

* revert to using a method to display the value indicator text instead of directly through the DOM * avoid styling based on tag name and instead target class names applied to host elements * fix indentation in slider.scss * condense jsdoc to oneline * expand MatSliderThumb jsdoc * reworded comment in MatSliderThumb * move initialization of _thumb value to property definition * move input value attribute initialization to separate method * move input min and max property initialization to separate method * move input value property initialization to separate method * remove unnecessary !

jelbourn

Overall LGTM aside from my last comment about the thumbPosition

…humb

…ngular#21844) * feat(material-experimental/mdc-slider): implement the SliderAdapter * complete the core logic for MatSliderThumb and MatSlider * collapse slider-thumb.ts and slider-adapter.ts into slider.ts

angular-automatic-lock-bot · 2021-03-20T00:10:03Z

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.}

feat(material-experimental/mdc-slider): implement the SliderAdapter

390b5f0

* complete the core logic for MatSliderThumb and MatSlider

wagnermaciel requested a review from devversion as a code owner February 8, 2021 23:09

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 8, 2021

wagnermaciel requested review from mmalerba and jelbourn February 8, 2021 23:12

mmalerba reviewed Feb 8, 2021

View reviewed changes

src/material-experimental/mdc-slider/slider-adapter.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider-adapter.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider-adapter.ts Outdated Show resolved Hide resolved

fix(material-experimental/mdc-slider): code review changes

29f119e

* delete commented out code * add comment explaining why emitChangeEvent and emitInputEvent are ignored

wagnermaciel requested a review from mmalerba February 9, 2021 01:46

crisbeto reviewed Feb 9, 2021

View reviewed changes

src/material-experimental/mdc-slider/slider-adapter.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider.ts Outdated Show resolved Hide resolved

jelbourn reviewed Feb 9, 2021

View reviewed changes

wagnermaciel added 2 commits February 11, 2021 11:23

fix(material-experimental/mdc-slider): code review changes

238ffea

* add todo to remember to add the mdc-slider back into the kitchen sink ssr app * move _foundation.init() into the _platform.isBrowser check to avoid ssr problems

wagnermaciel requested review from jelbourn and crisbeto February 11, 2021 19:51

mmalerba reviewed Feb 11, 2021

View reviewed changes

src/material-experimental/mdc-slider/slider-adapter.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider-adapter.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider-thumb.ts Outdated Show resolved Hide resolved

wagnermaciel requested a review from mmalerba February 12, 2021 16:14

crisbeto reviewed Feb 12, 2021

View reviewed changes

jelbourn reviewed Feb 12, 2021

View reviewed changes

wagnermaciel added 2 commits February 16, 2021 08:19

fix(material-experimental/mdc-slider): fix comments in slider-adapter

2a98bf7

* add class description jsdoc * move comment explaining why we use arrow fns

wagnermaciel force-pushed the slider-adapter branch from 4552321 to 2a98bf7 Compare February 16, 2021 17:08

wagnermaciel added 2 commits February 16, 2021 14:07

wagnermaciel requested review from jelbourn and crisbeto February 16, 2021 23:06

mmalerba approved these changes Feb 16, 2021

View reviewed changes

src/material-experimental/mdc-slider/slider.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider.ts Outdated Show resolved Hide resolved

src/material-experimental/mdc-slider/slider.ts Outdated Show resolved Hide resolved

fix(material-experimental/mdc-slider): code review changes

6e8ab85

* delete empty providers * rename thumb to _thumb and make it private in MatSliderThumb * delete thumb property in MatSliderDragEvent

jelbourn reviewed Feb 17, 2021

View reviewed changes

jelbourn approved these changes Feb 17, 2021

View reviewed changes

fix(material-experimental/mdc-slider): use thumbPosition instead of t…

0558d5b

…humb

wagnermaciel force-pushed the slider-adapter branch from a64bb8e to 0558d5b Compare February 17, 2021 20:07

wagnermaciel added the target: feature This PR is targeted for a feature branch (outside of main and semver branches) label Feb 17, 2021

wagnermaciel merged commit 124b7de into angular:mdc-slider Feb 17, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 20, 2021

feat(material-experimental/mdc-slider): implement the SliderAdapter #21844

feat(material-experimental/mdc-slider): implement the SliderAdapter #21844

Uh oh!

Conversation

wagnermaciel commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wagnermaciel commented Feb 8, 2021

Uh oh!

mmalerba commented Feb 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crisbeto Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

wagnermaciel Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jelbourn Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

wagnermaciel Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jelbourn Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

wagnermaciel Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jelbourn Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Mar 20, 2021

Uh oh!

wagnermaciel commented Feb 8, 2021 •

edited

Loading