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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RangeSlider] Fix range slider focus state #1926

Merged
merged 1 commit into from Jan 6, 2020
Merged

Conversation

spencercanner
Copy link
Contributor

@spencercanner spencercanner commented Aug 6, 2019

WHY are these changes introduced?

  • The range slider currently doesn't have a visible focus state, which is especially noticeable when it is used without the numeric output bubble

WHAT is this pull request doing?

  • Removed a class selector from the SingleThumb styles which was preventing the focus styles from being applied to the input
    • The .RangeSlider selector was never used anywhere in the component, and was left over from the before the DualThumb component existed
  • Added an outline to the entire SingleThumb input when focused in high contrast mode
  • Updated the SingleThumb styles to prevent focus from taking over the error track colour
    • This should improve consistency across other input components (e.g. checkbox, select), where it will maintain some error styles but the focused area will receive an indigo outline
  • Copied over the focus styles to the DualThumb scss to apply consistent focus styles across both components
  • Added an outline to the DualThumb thumb button when focused in high contrast mode
  • Disabled the DualThumb buttons to prevent them from receiving focus when the component is disabled
    • The single thumb range slider cannot receive focus in a disabled state, so this improves consistency across range sliders

How to 馃帺

馃枼 Local development instructions
馃棐 General tophatting guidelines
馃搫 Changelog guidelines

General case:

  • Tab into the input, and verify the slider receives an indigo border
  • Use the mouse to move the slider, and verify it receives the focus state

Error state:

  • Update component to be in an error state by adding error="error" as a prop
  • Verify that focus states will not change the track colour, but the slider will receive an indigo border
Copy-paste this code in playground/Playground.tsx:
import React, {useCallback, useState} from 'react';
import {Page, Card, RangeSlider} from '../src';

export function Playground() {
  const initialValue = [700, 1000];
  const prefix = '$';
  const min = 0;
  const max = 2000;
  const step = 10;

  const [doubleRangeValue, setDoubleRangeValue] = useState(initialValue);
  const [rangeValue, setRangeValue] = useState(32);

  const handleDoubleRangeSliderChange = useCallback((value) => {
    setDoubleRangeValue(value);
  }, []);

  const handleRangeSliderChange = useCallback(
    (value) => setRangeValue(value),
    [],
  );

  return (
    <Page title="Playground">
      <Card sectioned title="Background color">
        <RangeSlider
          label="Opacity percentage"
          value={rangeValue}
          onChange={handleRangeSliderChange}
          output
        />
      </Card>
      <Card sectioned title="Minimum requirements">
        <RangeSlider
          output
          label="Money spent is between"
          value={doubleRangeValue}
          prefix={prefix}
          min={min}
          max={max}
          step={step}
          onChange={handleDoubleRangeSliderChange}
        />
      </Card>
    </Page>
  );
}

Mac Chrome:
Single without focus:
image
Single with focus:
image
Single error without focus:
image
Single error with focus:
image

Double without focus:
image
Double with focus:
image
Double error without focus:
image
Double error with focus:
image

Edge:
Single without focus:
image
Single with focus:
image
Single error without focus:
image
Single error with focus:
image

Double without focus:
image
Double with focus:
image
Double error without focus:
image
Double error with focus:
image

High contrast mode:
Single without focus:
image
Single with focus:
image
Single error without focus:
image
Single error with focus:
image

Double without focus:
image
Double with focus:
image
Double error without focus:
image
Double error with focus:
image

馃帺 checklist

@ghost
Copy link

ghost commented Aug 6, 2019

馃憢 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven鈥檛 already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@BPScott BPScott temporarily deployed to polaris-react-pr-1926 August 6, 2019 18:50 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1926 August 6, 2019 19:05 Inactive
@danrosenthal
Copy link
Contributor

Hey @spencercanner, sorry for the delay on getting review on this PR, seems like it must have fallen between the cracks.

I've tophatted this change, and it's looking great in the single input range slider. I'd love to see this change made in the dual input range slider as well. The focus state in it currently is pretty gross.

Dual Input
Screen Shot 2019-09-25 at 10 49 25 AM

Also pinging @dpersing for her thoughts on the high contrast styles. I wonder if the button-text-background color would be more appropriate here. It'd also be great to add high contrast background color to the track so it's visible. But that could be done in another PR if you like.

@dpersing
Copy link
Contributor

@danrosenthal @spencercanner Hi! Thanks so much for catching this.

I think we want to maintain a focus state for High Contrast that doesn't rely on color alone, since it's a subtle change. Having a focus indicator on the full input would be ideal, since this mimics the default focus ring behavior.

What tool and browser are you using for the high contrast setting in this case? I ask since we typically test with Edge and the native High Contrast theme settings on Windows, which looks like this for the current example in the style guide:

Default:

Slider in High Contrast

Focus state:

Slider in High Contrast with focus indicator

@spencercanner
Copy link
Contributor Author

@dpersing I was testing with Edge as well, but running it on a VirtualBox Windows VM using the High Contrast Black theme. I just checked the polaris styleguide, and I'm seeing this as the focus state:
image
Maybe the VM isn't providing a good representation of an actual machine?

@dpersing
Copy link
Contributor

@spencercanner Gotcha, that is curious. I had the same theme, but I'm using VMware and looking at the version that's currently in the style guide. I'll block out some time later today to take a closer look.

@dpersing
Copy link
Contributor

dpersing commented Sep 26, 2019

Yeah, here's your version in VMware in High Contrast black.

High Contrast black theme on Windows 10 in VMware

馃槱

I'd still like the err on the side of reproducing the default focus ring around the whole slider in High Contrast if we can. This more closely matches the default-style focus ring in the dual-thumb version.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2019

馃挦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified5
Files potentially affected3

Details

All files potentially affected (total: 3)
馃搫 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

馃帹 src/components/RangeSlider/components/DualThumb/DualThumb.scss (total: 2)

Files potentially affected (total: 2)

馃З src/components/RangeSlider/components/DualThumb/DualThumb.tsx (total: 1)

Files potentially affected (total: 1)

馃З src/components/RangeSlider/components/DualThumb/tests/DualThumb.test.tsx (total: 0)

Files potentially affected (total: 0)

馃帹 src/components/RangeSlider/components/SingleThumb/SingleThumb.scss (total: 2)

Files potentially affected (total: 2)


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@spencercanner
Copy link
Contributor Author

@danrosenthal I've updated the PR to include the single and dual thumb range slider focus states, and added more detail in the PR description above

@dpersing I think the HC inconsistencies were caused by my VM running an older version of Edge, but I've updated the PR to include HC focus styling for both the single and dual thumb range sliders

Copy link
Contributor

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

Hey @spencercanner, sorry this one fell between the couch cushions. I'm happy for this to merge as-is once it's gotten a rebase. @dpersing any objections?

@danrosenthal
Copy link
Contributor

@spencercanner, I'm going to go ahead and rebase, fix the conflict, and merge this PR.

@danrosenthal danrosenthal merged commit 47888ac into master Jan 6, 2020
@danrosenthal danrosenthal deleted the range-slider-focus branch January 6, 2020 15:19
@ghost
Copy link

ghost commented Jan 6, 2020

馃帀 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.

None yet

4 participants