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

Helping with dual range input props #2

Conversation

cchaos
Copy link

@cchaos cchaos commented Jan 8, 2020

I'll comment in the diff the changes I've made.

@@ -1,6 +1,6 @@
## [`master`](https://github.com/elastic/eui/tree/master)

- Added possibility to set `aria-label` for both `min` and `max` inputs of `EuiDualRange` ([#2738](https://github.com/elastic/eui/pull/2738))
- Added `minInputProps` and `maxInputProps` to supply more props to the inputs of `EuiDualRange` ([#2738](https://github.com/elastic/eui/pull/2738))
Copy link
Author

Choose a reason for hiding this comment

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

Just made this more generic since it's no longer just about aria-label

// Overridable props
aria-describedby={this.props['aria-describedby']}
aria-label={this.props['aria-label']}
{...minInputProps}
Copy link
Author

Choose a reason for hiding this comment

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

This is the order that I was trying to mention before. All the props below this line are mandator and therefore cannot be allowed to be overridden by ...inputProps. The only way to ensure this is that ...inputProps comes before the mandator props.

@@ -176,56 +176,5 @@ describe('EuiDualRange', () => {

expect(component).toMatchSnapshot();
});

test('should be set only for the min input', () => {
Copy link
Author

Choose a reason for hiding this comment

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

This and the tests below were still too specific for aria-label, and really just ensuring that the input props get passed down, doesn't error out, is all that's needed here so I simplified the version above and removed these and below.

@ffknob ffknob merged commit 25a7e5a into ffknob:accesibility-EuiDualRange-aria-labels Jan 8, 2020
@cchaos cchaos deleted the accesibility-EuiDualRange-aria-labels/cchaos branch January 8, 2020 22:18
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.

2 participants