Skip to content

prototype(slider): create prototype slider based on MDC web #16795

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

Merged
merged 2 commits into from
Aug 29, 2019

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 16, 2019

Creates a prototype of mat-slider that uses MDC web. The feature comparison document has been filled.

A list of notable bugs in the MDC foundation right now:

  • input event not rounded before comparing (causing multiple input events; workaround applied)
  • Thumb label not updating if value changes programmatically (workaround applied)
  • Slider updates value with right mouse events
  • change event always fired on mouse up or keydown
  • slider measurements and ticks only updated on init and "resize" event
    • standard Angular Material slider did this on mouseenter.
  • Vertical alignment (incorrect calculations; currently shifted down 2px of baseline)

Notable feature differences:

  • Visually sliders look totally different to our current implementation
  • Vertical sliders are not supported (yet?)
  • Sliders do not support decimal steps.
  • Sliders cannot be inverted (no support in the foundation)

Currently blocked on #16772.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 16, 2019
@devversion devversion force-pushed the feat/mdc-slider branch 5 times, most recently from 6d263de to e2e74b0 Compare August 16, 2019 11:51
@mmalerba
Copy link
Contributor

I think MDC is planning to redo the slider at some point because the visual style has updated in the spec. If they're planning to also revisit the API and supported features at that time it may make sense to wait on this one.

@abhiomkar do you know if there are any plans to change the API when updating the visual style?

@devversion
Copy link
Member Author

@mmalerba I had a look at their slider branch which I assumed was for the mentioned rework and it looked like most of the things remained the same and just a few public-facing foundation/adapter changes were done.

Happy to wait for the rework to land though. I just didn't think the rework will happen anytime soon and if it lands, we can re-use this work as most of the things should remain the same.

https://github.com/material-components/material-components-web/commits/feat/slider (last commit Aug 2018; unless there is a different branch)

@abhiomkar
Copy link

Slider refactor is not planned in this quarter.

@mmalerba
Copy link
Contributor

Ok, sounds like we can just go ahead with this for now

@devversion devversion force-pushed the feat/mdc-slider branch 2 times, most recently from 2f295ba to a34ca48 Compare August 16, 2019 18:07
@devversion devversion added the target: major This PR is targeted for the next major release label Aug 16, 2019
});

// TODO: MDC slider does not respect modifier keys.
// tslint:disable-next-line
Copy link
Member

Choose a reason for hiding this comment

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

File a bug on their repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it's by intention that they don't respect modifier keys. Native range sliders don't seem to respect these either (i.e. value still changes with SHIFT + LEFT ARROW).

How should we proceed? based on their implementation of the keyboard logic we can't overwrite/configure this behavior.

@devversion devversion added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 20, 2019
@devversion
Copy link
Member Author

@jelbourn Feedback addressed. Created the issues on the MDC repository and updated the feature comparison doc accordingly.

@mmalerba mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 23, 2019
@mmalerba
Copy link
Contributor

Should be unblocked now that the chips changes are in

@devversion
Copy link
Member Author

Yes. thanks @mmalerba for updating the label.

@devversion
Copy link
Member Author

@jelbourn can you have another look at this PR when you get a chance?

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, minor comments

@devversion devversion requested a review from a team as a code owner August 29, 2019 12:09
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 action: merge The PR is ready for merge by the caretaker pr: merge safe labels Aug 29, 2019
@jelbourn jelbourn merged commit ff94f9d into angular:master Aug 29, 2019
@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 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants