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

feat(slider): Migrate to custom slider due to range input limitations #1410

Merged
merged 2 commits into from
Jul 16, 2021

Conversation

jstoffan
Copy link
Collaborator

No description provided.

@jstoffan jstoffan requested a review from a team as a code owner July 14, 2021 23:53
if (!sliderEl) return 0;

const { width: sliderWidth } = sliderEl.getBoundingClientRect();
const newValue = (getPosition(pageX) / sliderWidth) * max;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to worry about decimal values here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we'd want to let the consuming component handle rounding to avoid making assumptions about its use. Videos can have durations defined as decimal values, for whatever reason, for example.

}, []);

const getPositionValue = React.useCallback(
(pageX: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does tslint not care about the return types for these anonymous functions used in useCallback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't. It looks like the return value is being inferred correctly in this case, though, so maybe that's why?

setIsScrubbing(true);
};

React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any issues with this when doing a quick click and drag and release interaction with the slider?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't seen any issues in my testing, but will keep an eye out as we build more features with this component.

@mergify mergify bot merged commit 19722e6 into box:master Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants