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.

Sign up for GitHub

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

add onChanged prop on Slider (#6108) #6127

Merged
merged 1 commit into from Sep 4, 2018
Merged

add onChanged prop on Slider (#6108) #6127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/slider-onBlur_2018-08-29-14-33.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "add onBlur on Slider",
"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "[email protected]"
}
6 changes: 5 additions & 1 deletion packages/office-ui-fabric-react/src/components/Slider/Slider.base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,12 +252,16 @@ export class SliderBase extends BaseComponent<ISliderProps, ISliderState> implem
);
}

private _onMouseUpOrTouchEnd = (): void => {
private _onMouseUpOrTouchEnd = (event: MouseEvent | TouchEvent): void => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work without modifying lines 167 and 170? Maybe i just don't know how that _events.on callback takes in props.

Last thing then, can you add a quick test to make sure this onChanged returns the event and correct number?

Copy link
Contributor Author

@blandine blandine Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to add a test previously but I didn't succeed to simulate a mousedown and then a mouseup to trigger the onChanged.
I followed this issue enzymejs/enzyme#426 and this one : enzymejs/enzyme#1357 saying that we should not use Enzyme simulate anymore.
Could you give me some pointers?
Thanks

Copy link
Member

Choose a reason for hiding this comment

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

For now, i'd just piggyback off of the existing onChange test and make sure your onChanged callback is being fired as well (including event). I agree, it seems enzyme is moving away from simulate, so no need to do anything elaborate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't work since onChanged is fired only when the mouseup or touchend happens.
On mousedown event (like in the extisting onChange test), onChanged won't be triggered.
I try to hack something like in the callout.test.tsx but it doesn't work :

const mouseEvent = document.createEvent('MouseEvent');
    const root = document.createElement('div');
    document.body.appendChild(root);

    ReactDOM.render<HTMLDivElement>(<Slider onChanged={onChanged} />, root);

    mouseEvent.initMouseEvent('mousedown', false, false, window, 0, 0, 0, 0, 0, false, false, false, false, 1, root);
    mouseEvent.initMouseEvent('mouseup', false, false, window, 0, 0, 0, 0, 0, false, false, false, false, 1, root);

Thank you for your time but I'm afraid I won't be able to complete this PR :/

// Synchronize the renderedValue to the actual value.
this.setState({
renderedValue: this.state.value
});

if (this.props.onChanged) {
this.props.onChanged(event, this.state.value as number);
}

this._events.off();
};

Expand Down
5 changes: 5 additions & 0 deletions packages/office-ui-fabric-react/src/components/Slider/Slider.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ export interface ISliderProps extends React.Props<SliderBase> {
*/
onChange?: (value: number) => void;

/**
* Callback on mouse up or touch end
*/
onChanged?: (event: MouseEvent | TouchEvent, value: number) => void;

/**
* A description of the Slider for the benefit of screen readers.
*/
Expand Down