Skip to content

feat(material-experimental): add test harness for slider #16688

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 1 commit into from
Aug 16, 2019

Conversation

devversion
Copy link
Member

No description provided.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 5, 2019
@devversion devversion force-pushed the feat/slider-harness branch from a2b773f to 51e4daa Compare August 5, 2019 11:53
@devversion devversion added pr: merge safe target: patch This PR is targeted for the next patch release labels Aug 5, 2019
@devversion
Copy link
Member Author

devversion commented Aug 5, 2019

@jelbourn @mmalerba I wasn't too sure how we can solve the setValue thing. There are two options:

  1. Setting the value through click (this is what currently is implemented as part of 51e4daa)
    • Can deviate in rare cases since not each value maps to a single pixel (that we an click).
  2. Setting the value by pressing the up or down arrow keys. This is unpredictable since the keys always cause a change based on the specified mat-slider#step.
    • Developer could specify a value that can't be selected through keyboard but got set through binding for example.

If we want to spend some more time exploring the options, I'd propose that we do this as a follow-up. That's why the logic for setting a value is in a separate commit.

@devversion devversion force-pushed the feat/slider-harness branch 3 times, most recently from 395344e to 00c1d69 Compare August 5, 2019 16:42
@mmalerba
Copy link
Contributor

mmalerba commented Aug 5, 2019

Ah that's interesting... I guess a real user would have similar difficulty using your slider if you didn't have it set so that all values can be reached via the keyboard or had more steps than pixels worth of track.

I guess I would say go for the clicking approach like you've done and maybe document that if you have more steps than pixels the setValue could be slightly off, but also that it would be a bad experience for your users and you should change it

@devversion devversion force-pushed the feat/slider-harness branch from 00c1d69 to d0c8d50 Compare August 5, 2019 19:10
@devversion
Copy link
Member Author

@mmalerba Done. I'm wondering if we should consider providing a method for selecting a value through keyboard interaction too. The arrow keys allow granular control over the value since those allow changes by "one step value" but in order to provide meaningful methods.. we'd need to somehow determine the step value from within the harness (which requires some host bindings on the slider..). I think that's something we could explore in a follow-up PR.

@devversion devversion force-pushed the feat/slider-harness branch from d0c8d50 to 1169198 Compare August 13, 2019 15:16
@jelbourn
Copy link
Member

I also agree that methods to increment/decrement by the step via keyboard would be good in a follow-up.

@devversion devversion force-pushed the feat/slider-harness branch from 1169198 to 7e8214d Compare August 15, 2019 09:53
@devversion
Copy link
Member Author

@jelbourn @mmalerba Addressed feedback.

@devversion devversion force-pushed the feat/slider-harness branch from f050b04 to fdbffa0 Compare August 16, 2019 08:43
@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Aug 16, 2019
@mmalerba mmalerba merged commit 0a16e04 into angular:master Aug 16, 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 16, 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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants