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

Slider Convergence #18886

Closed
23 of 40 tasks
khmakoto opened this issue Jul 9, 2021 · 5 comments · Fixed by #19437, #19631, #19696 or #21506
Closed
23 of 40 tasks

Slider Convergence #18886

khmakoto opened this issue Jul 9, 2021 · 5 comments · Fixed by #19437, #19631, #19696 or #21506

Comments

@khmakoto
Copy link
Member

khmakoto commented Jul 9, 2021

Preparation:

Implementation

link to the package

  • Started impl
  • Implement component
  • Add storybook stories
  • Using hooks
  • Using makeStyles
  • Respects Figma tokens (and using provider)
  • Respects API principles, shorthands and slots handling
  • No dependency on v0/v7
  • Add tests - Conformance, Unit, and VR
    • Conformance tests
    • Unit tests
    • VR tests
    • Accessibility behavior tests
  • Write README.md covering basic usage
  • Write initial MIGRATION.md guide (include v8 and v0)
  • Move utils to react-utilities package
  • Set component ownership in CODEOWNERS
  • Add RangedSlider
  • Add useEvent and useCapture and replace on
  • Set the hidden input element to the primary slot
  • Deliverable: Experimental component ready for partner use, component re-exported in react-components

Future Investigation

Validation

  • Started validating
  • Add and validate in UI Builder
  • Add and validate in docs site
  • Validate with token pipeline
  • Validate in product
  • Finalize migration guide
    • Author codemods
  • Deliverable: Preview component ready for broader/3rd party use
@behowell
Copy link
Contributor

behowell commented Aug 2, 2021

8-2 update - @czearing:

  • Basic implementation is out in PR
  • Updating styling, useSlider API
  • Working on implementing marks
  • Investigating RangeSlider

@behowell
Copy link
Contributor

behowell commented Aug 9, 2021

8-9 update @czearing

  • Spec is merged!
  • Basic implementation merged!
  • PRs out for vertical prop and disabled prop
  • Working on step and snap functionality
    • Open questions about rounding and animations

@behowell
Copy link
Contributor

@ecraig12345
Copy link
Member

@czearing Some issues I noticed belatedly with onChange, while reviewing another PR:

  • onChange should have type (ev: React.PointerEvent<HTMLDivElement>, data: { value: number }) => void as discussed in one of the RFCs. (If the event is sometimes not present, you can add | undefined to the type.)
  • In useSliderState, the onChangeCallback ref is never being updated. After the ref is defined, you should add a line onChangeCallback.current = onChange to update it every render.
  • Once you start updating the ref, the updateValue implementation won't need to directly reference onChange; instead it can just use the ref. (Same in useMount.)
    if (onChange && onChangeCallback.current) {
    onChangeCallback.current(clampedValue, ev);
    }
    setCurrentValue(clampedValue);
    },
    [max, min, onChange, setCurrentValue],

@msft-fluent-ui-bot msft-fluent-ui-bot added Status: Fixed Fixed in some PR and removed Status: In PR labels Sep 8, 2021
@czearing czearing reopened this Sep 8, 2021
@czearing czearing removed the Status: Fixed Fixed in some PR label Sep 14, 2021
@ecraig12345 ecraig12345 assigned micahgodbolt and unassigned czearing Nov 4, 2021
@msft-fluent-ui-bot msft-fluent-ui-bot added Status: Fixed Fixed in some PR and removed Status: In PR labels Feb 2, 2022
@micahgodbolt micahgodbolt reopened this Feb 17, 2022
@microsoft microsoft locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.