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(foundation): remove readonly from Slider #6481

Closed
wants to merge 9 commits into from

Conversation

olaf-k
Copy link
Contributor

@olaf-k olaf-k commented Oct 28, 2022

πŸ“– Description

This PR removes the readonly state for the Slider component in an effort to keep it close to the input range's behavior.
The specs for the latter can be found here (see the 'Bookkeeping details' part after the examples).
image

See PR #6438 for a similar update to the radio button.

πŸ‘©β€πŸ’» Reviewer Notes

As it is a removal, the change is pretty straightforward. Code/tests/doc should be covered.
slider-label should not be affected as far as I can tell.

βœ… Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

@olaf-k
Copy link
Contributor Author

olaf-k commented Oct 28, 2022

@microsoft-github-policy-service agree company="Vonage"

Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

This PR seems very similar in nature to the one we have for radio groups that makes the same set of changes. Like that one, I want to make sure we don't have a bug in the key handler. Shouldn't it be checking for disabled state like the mouse handler or does the platform handle that automatically?

@olaf-k
Copy link
Contributor Author

olaf-k commented Mar 24, 2023

hey team! do you think it'd be possible to push this forward to complete our great ReadOnly Purge? 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants