SwatchColorPicker: Adding Aria Label Text#3765
Conversation
…es to be more generic
| getStyles, | ||
| } = this.props; | ||
|
|
||
| const htmlProps = getNativeProps(this.props, htmlElementProperties, ['data']); |
There was a problem hiding this comment.
is there an overridden data prop on SwatchColorPicker? If not, then you don't need the third param
There was a problem hiding this comment.
Do we even need to get the native props here? Couldn't we just spread the props we were given until we get into Grid and then get the nativeProps. Note: you'd want to make sure any of the props that map to native html attributes we want overriden or that we are changing props as part of the creation of the Grid in the SwatchColorPicker to be defined after the spread (e.g. items, onBlur, and theme for example). To keep it clean, it's best to do the spread first and then have all of our specific props to override them
There was a problem hiding this comment.
Got it, there were no particular reason for this. I made an assumption that it was needed based off the ResizeGroup PR, but you're right, it's not needed.
| columnCount={ columnCount } | ||
| onRenderItem={ this._renderOption } | ||
| positionInSet={ positionInSet && positionInSet } | ||
| setSize={ setSize && setSize } |
There was a problem hiding this comment.
Why did these get move in front of htmlProps? If it's because those are getting return as part of the htmlProps, then you might want to consider including those attributes in the exclude list in the getNativeProps call
There was a problem hiding this comment.
Will do, I'll exclude it. I was thinking that we should no longer set these types manually and was trying to push to use the native supported for aria attributes.
| getStyles, | ||
| } = this.props; | ||
|
|
||
| const htmlProps = getNativeProps(this.props, htmlElementProperties, ['aria-posinset, aria-setsize']); |
There was a problem hiding this comment.
Do you need this here, could you just pass { ...this.props } to Grid and then do the trimming there?
2 participants
Description of changes
Currently our swatch color picker doesn't have an option to enable aria-label text. Added support to pass in an aria-label from the SwatchColorPicker and the Grid component that is being used as a base.
Focus areas to test
Swatch Color Picker page.