Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(slider): v1 #1559

Merged
merged 16 commits into from
Jul 12, 2019
Merged

feat(slider): v1 #1559

merged 16 commits into from
Jul 12, 2019

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Jun 28, 2019

feat(slider): v1

Closes #1539

Description

This PR consists of the first version of the Slider component. It incorporates:

1. Functionality (demo below):

Screen Recording 2019-06-28 at 07 59 34

2. Styling (according to design files and redlines for all themes):

Theme Name Slider flavor
Base Screenshot 2019-06-28 at 08 05 07
Teams Screenshot 2019-06-28 at 07 58 09
Teams Dark Screenshot 2019-06-28 at 07 58 25
Teams HC Screenshot 2019-06-28 at 07 58 44

3. Accessibility:

  • leverages native keyboard navigation
  • adds correct accessibility attributes via the sliderBehavior

4. Doc examples and a few visual tests

API

export interface SliderProps {
  /**
   * Accessibility behavior if overridden by the user.
   * @default sliderBehavior
   */
  accessibility?: Accessibility

  /** The default value of the slider. */
  defaultValue?: SupportedIntrinsicInputProps['defaultValue'] // TODO 'value' type is React.ReactText but it doesn't work for 'defaultValue'

  /**
   * A slider can be read-only and unable to change states.
   * @default false
   */
  disabled?: SupportedIntrinsicInputProps['disabled']

  /**
   * A slider can take the width of its container.
   * @default false
   */
  fluid?: boolean

  /**
   * Callback that creates custom accessibility message a screen reader narrates when the value changes.
   * @param {SliderProps} props - Slider props.
   */
  getA11yValueMessageOnChange?: (props: SliderProps) => string

  /** Shorthand for the input component. */
  input?: ShorthandValue

  /** Ref for input DOM node. */
  inputRef?: React.Ref<HTMLElement>

  /**
   * The minimum value of the slider
   * @default 0
   */
  min?: SupportedIntrinsicInputProps['min']

  /**
   * The maximum value of the slider.
   * @default 100
   */
  max?: SupportedIntrinsicInputProps['max']

  /**
   * Called after item checked state is changed.
   * @param {SyntheticEvent} event - React's original SyntheticEvent.
   * @param {object} data - All props.
   */
  onChange?: ComponentEventHandler<SliderProps>

  /**
   * A number that specifies the granularity that the value must adhere to, or the special value 'any'.
   * A string value of any means that no stepping is implied, and any value is allowed
   * (barring other constraints, such as min and max).
   * @default 1
   */
  step?: SupportedIntrinsicInputProps['step']

  /** The value of the slider. */
  value?: React.ReactText

  /**
   * A slider can be displayed vertically.
   * @default false
   */
  vertical?: boolean
}

TODO

  • Focus borders: still didn't find a way to add double borders to the slider's thumb
  • Remove slider marks in IE
  • Improve perf for recalculating props (accessibility props, styling props, etc) with every change of the slider's value
  • Add more visual tests

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Oops, CI fails and also there is warning somewhere is examples:
image

@bmdalex bmdalex force-pushed the feat/slider branch 3 times, most recently from 5fc56ed to b1bb1c6 Compare July 2, 2019 12:16
@bmdalex
Copy link
Collaborator Author

bmdalex commented Jul 2, 2019

Oops, CI fails and also there is warning somewhere is examples:
image

@layershifter fixed...but it wasn't clear at all how to do this so I created #1571 to address that; pls provide your feedback

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #1559 into master will decrease coverage by 0.21%.
The diff coverage is 51.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   71.46%   71.24%   -0.22%     
==========================================
  Files         845      851       +6     
  Lines        6945     7021      +76     
  Branches     1981     2020      +39     
==========================================
+ Hits         4963     5002      +39     
- Misses       1976     2013      +37     
  Partials        6        6
Impacted Files Coverage Δ
packages/react/src/index.ts 100% <ø> (ø) ⬆️
packages/react/src/lib/htmlPropsUtils.tsx 100% <ø> (ø) ⬆️
...ackages/react/src/components/Checkbox/Checkbox.tsx 80% <ø> (ø) ⬆️
...c/themes/base/components/Slider/sliderVariables.ts 0% <0%> (ø)
...ges/react/src/themes/teams/getBorderFocusStyles.ts 15% <0%> (-3.75%) ⬇️
.../themes/teams/components/Slider/sliderVariables.ts 0% <0%> (ø)
packages/react/src/components/Input/Input.tsx 100% <100%> (ø) ⬆️
.../accessibility/Behaviors/Slider/sliderBehavior.tsx 100% <100%> (ø)
.../src/themes/base/components/Slider/sliderStyles.ts 20% <20%> (ø)
...src/themes/teams/components/Slider/sliderStyles.ts 33.33% <33.33%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c194ef...4ea9de6. Read the comment docs.

@bmdalex bmdalex force-pushed the feat/slider branch 2 times, most recently from 07b4da8 to fc0d54d Compare July 8, 2019 12:55
@vercel
Copy link

vercel bot commented Jul 10, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://stardust-react-git-feat-slider.stardust-ui.now.sh

@bmdalex
Copy link
Collaborator Author

bmdalex commented Jul 12, 2019

RTL is broken :-(
slider-rtl

Both mouse and keyboard.

RTL doesn't seem to work as expected:
LTR:
image
RTL:
image

@mnajdova
@miroslavstastny

great catch, thanks! RTL was fixed and I also added an example for it in the docs. Please check again.

@bmdalex
Copy link
Collaborator Author

bmdalex commented Jul 12, 2019

Disabled slider with icon does not look correct (the icon):
image

@miroslavstastny I think you were testing an earlier version 😕 removed icon and iconPosition altogether from Slider

@vercel vercel bot temporarily deployed to staging July 12, 2019 12:38 Inactive
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

👍

@bmdalex bmdalex merged commit e8ce890 into master Jul 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/slider branch July 12, 2019 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
architecture new component Component planned to be added to Stardust 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slider component
5 participants