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

[DualThumb] Add an offset to push the minimum range value to the left #4172

Merged
merged 1 commit into from
May 12, 2021

Conversation

mathildebuenerd
Copy link
Contributor

@mathildebuenerd mathildebuenerd commented May 6, 2021

WHY are these changes introduced?

Fixes #4155

When using the Dual thumb range slider with a min value different from 0, there is an extra padding on the left. This prevents selecting a value when having a range [10, 50] for example:

Screenshot 2021-05-06 at 11 10 43

Screenshot 2021-05-06 at 11 09 26

What's happening What's expected
current expected

WHAT is this pull request doing?

Substract the minimum range value position to the position.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Page} from '../src';
import {DualThumb} from '../src/components/RangeSlider/components/DualThumb';

export function Playground() {
  const handleChange = (value) => value;
  return (
    <Page title="Playground">
      <DualThumb
        id="test"
        label="Range=[0,50] - Value=[10,50]"
        min={0}
        max={50}
        value={[10, 50]}
        step={1}
        onChange={handleChange}
      />
      <DualThumb
        id="test"
        label="Range=[10,50] - Value=[10,50]"
        min={10}
        max={50}
        value={[10, 50]}
        step={1}
        onChange={handleChange}
      />
      <DualThumb
        id="test"
        label="Range=[10,50] - Value=[20,40]"
        min={10}
        max={50}
        value={[20, 40]}
        step={1}
        onChange={handleChange}
      />
     <DualThumb
        id="test"
        label="Range=[-10,30] - Value=[-10,-9]"
        min={-10}
        max={30}
        value={[-10, -9]}
        step={1}
        onChange={handleChange}
      />
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2021

🟢 This pull request modifies 1 files and might impact 1 other files.

Details:
All files potentially affected (total: 1)
🧩 src/components/RangeSlider/components/DualThumb/DualThumb.tsx (total: 1)

Files potentially affected (total: 1)

@mathildebuenerd mathildebuenerd self-assigned this May 6, 2021
Copy link
Contributor Author

@mathildebuenerd mathildebuenerd left a comment

Choose a reason for hiding this comment

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

Not sure if I should add an entry to the changelog?

@mathildebuenerd mathildebuenerd marked this pull request as ready for review May 6, 2021 09:28
Copy link
Contributor

@LauraAubin LauraAubin left a comment

Choose a reason for hiding this comment

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

Tophat is working as expected!

I think you want to add a changelog entry under bug fixes

@mathildebuenerd mathildebuenerd force-pushed the 4155/bug-dual-thumb-range-slider branch from 3ebed21 to 0771eda Compare May 7, 2021 07:35
@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

size-limit report

Path Size
cjs 141.58 KB (+0.02% 🔺)
esm 95.4 KB (+0.03% 🔺)
esnext 138.4 KB (+0.02% 🔺)
css 33.6 KB (0%)

@mathildebuenerd
Copy link
Contributor Author

@LauraAubin cool thank you, I added it to unreleased.md

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Code looks good—would love to see a test added for this!

@mathildebuenerd mathildebuenerd force-pushed the 4155/bug-dual-thumb-range-slider branch from 0771eda to fdf1b7d Compare May 12, 2021 09:07
… to the left when minimum range value is different from 0
@mathildebuenerd mathildebuenerd force-pushed the 4155/bug-dual-thumb-range-slider branch from fdf1b7d to c19bde1 Compare May 12, 2021 09:21
@mathildebuenerd mathildebuenerd merged commit efd8d98 into main May 12, 2021
@mathildebuenerd mathildebuenerd deleted the 4155/bug-dual-thumb-range-slider branch May 12, 2021 14:12
@ghost
Copy link

ghost commented May 12, 2021

🎉 Thanks for your contribution to Polaris React!

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.

DualThumb slider does not behave as expected with min value above 0
3 participants