airbnb / rheostat

Rheostat is a www, mobile, and accessible slider component built with React
MIT License
1.69k stars 189 forks source link

aria-label and aria-valuetext #206

Closed catamphetamine closed 5 years ago

catamphetamine commented 5 years ago

I can see that <DefautlHandle/> does declare aria-label and aria-valuetext properties. https://github.com/airbnb/rheostat/blob/563f6a739023dbd3c77311323de109ae66c4ac7c/src/DefaultHandle.jsx#L13-L14

Still, <Handle/> doesn't get passed these two properties. https://github.com/airbnb/rheostat/blob/master/src/Slider.jsx#L803-L821

At the same time, there's handle property which is for React.Component. So I suppose a developer is supposed to pass their own handle component which must wrap the <DefaultHandle/>. Like this:

function CustomHandle(props) {
  return (
    <DefaultHandle
      {...props}
      aria-label={props['data-handle-key'] === 0 ? 'Minimum price' : 'Maximum price'}
      aria-valuetext={'$' + props['aria-valuenow']}/>
  )
}

Is this the recommended approach to add aria-label and aria-valuetext? If yes then is DefaultHandle being exported from the library or should it be imported from as rheostat/src/DefaultHandle.jsx?

ljharb commented 5 years ago

You don’t have to wrap the default handle; you could certainly wrap your own component that had aria values hardcoded.

You don’t want to import from src, but yes, a deep import of the default handle is fine.

catamphetamine commented 5 years ago

@ljharb

You don’t have to wrap the default handle; you could certainly wrap your own component that had aria values hardcoded.

Doesn't make a lot of sense to re-implement the handle. Contrary, it would be considered a bad practice. The correct way is definitely extending the default one.

You don’t want to import from src.

But everyone does because your library exports files from src as the main ones. So, contrary, you will be importing from src. https://github.com/airbnb/rheostat/blob/1fc67be62f3f0f61eee1838b7e186756a8c9dd13/package.json#L5-L6

but yes, a deep import of the default handle is fine.

I'd say it's the only way with the current situtation of handle components not being exported from the library.

If you want to know how to make the accessibility API better then you can introduce two new properties on the main component: aria-label and aria-valuetext. aria-valuetext will be passed through as is to the handles. aria-label will be an array and props['aria-label'][idx] will be passed through to the respective handle.

catamphetamine commented 5 years ago

That works, closing.

ljharb commented 5 years ago

main exports from lib; that’s what I’d recommend using.

droda59 commented 5 years ago

@catamphetamine Can you explain what worked, exactly? If I pass a component to the handle prop of the slider, every other prop I pass to the slider are not used. For example, if I pass a min/max to the slider everything is fine, but the moment I define a handle (even using your snippet at the top), the slider uses the default min/max, there's no snapPoints anymore.

So were you able to use your own handle, and use your own aria labels? Thanks!

catamphetamine commented 5 years ago

@droda59 I don't know what exactly worked, but something surely did.

import Rheostat from 'rheostat';

        <Rheostat
          snap={snap}
          handle={currency === 'USD' ? PriceHandleUSD : Handle}
          min={min}
          max={max}
          values={[currentRefinement.min, currentRefinement.max]}
          onChange={this.onChange}
          onValuesUpdated={this.onValuesUpdated}
          disabled={this.props['aria-hidden'] ? true : undefined} />

function PriceHandleUSD(props) {
  return (
    <Handle
      {...props}
      aria-label={props['data-handle-key'] === 0 ? 'Minimum price' : 'Maximum price'}
      aria-valuetext={'$' + props['aria-valuenow']} />
  );
}

function Handle(props) {
  return (
    <button
      type="button"
      aria-label={props['data-handle-key'] === 0 ? 'From' : 'To'}
      {...props}
      tabIndex={props['aria-disabled'] ? -1 : props.tabIndex} />
  );
}