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

Make switch use a random id if none is provided #779

Merged
merged 5 commits into from
Jul 19, 2018

Conversation

snide
Copy link
Contributor

@snide snide commented May 4, 2018

Fixes #748
Fixes #1027

Switches now generate a random id if none are passed (similar to how we do it in EuiFormRow. This should only happen when used on their own, since most other situations will usually generate one.

This seemed like the best solution since I could not require ids to be provided, otherwise it would spit out errors when EuiFormRow cloned ids down.

@@ -7,7 +7,7 @@ import { EuiSwitch } from './switch';
describe('EuiSwitch', () => {
test('is rendered', () => {
const component = render(
<EuiSwitch {...requiredProps} />
<EuiSwitch id="test" {...requiredProps} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can mock the makeId function; having two tests, one where you pass id and one where id is generated, would provide the best coverage of the component's id logic.

@timroes
Copy link
Contributor

timroes commented May 7, 2018

This PR shows really, that someone should look into the whole htmlIdGenerator vs makeId mess. (#639, #637) and basically merge them again into one generator. Because htmlIdGenerator is the one we use all over Kibana to generate HTML ids, but within EUI some components seem to import makeId like this PR from the row component.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Didn't look at the code but I tested this in VoiceOver in Chrome and it works as expected.

@snide
Copy link
Contributor Author

snide commented Jul 19, 2018

Updated / rebased. I'll make an issue to update the test to be more intense, but merge this for now since it's been hanging for awhile.

@snide snide merged commit e91d433 into elastic:master Jul 19, 2018
@snide snide deleted the accessibility/switch branch July 19, 2018 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants