-
Notifications
You must be signed in to change notification settings - Fork 64
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
Slider README #229
Slider README #229
Conversation
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
======================================
Coverage 99.1% 99.1%
======================================
Files 23 23
Lines 1334 1334
Branches 401 401
======================================
Hits 1322 1322
Partials 12 12 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor feedback, looks great overall!
src/slider/README.md
Outdated
# Slider widget | ||
# @dojo/widgets/slider/Slider widget | ||
|
||
Dojo 2's `Slider` widget creates a range slider control with a styleable track, fill, and thumb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will everyone know what a thumb is?
src/slider/README.md
Outdated
|
||
### Accessibility Features | ||
|
||
`Slider` uses the native `<input type="range">`as its base, which ensures built-in keyboard and screen reader accessibility. All common form field attributes (`disabled`, `invalid`, `readOnly`, `required`) may be set, as well as a visible or hidden label. The output node is associated with the input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The output node is associated with the input." I'm not sure most readers will know what is meant by this.
|
||
The following CSS classes are available on the `Slider` widget for use with custom themes: | ||
|
||
- `root`: Applied to either the wrapping `<label>`, or a `<div>` in the same position in the node hierarchy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not essential but it might be nice to organize these into two sections (always, vs. conditional)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
Type: feature
The following has been addressed in the PR:
Description:
Resolves #189, adds Slider readme and fixes event typings on the example page.