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

Adds getAriaName function and applies it to advanced settings #13448

Merged
merged 1 commit into from
Aug 16, 2017

Conversation

BigFunger
Copy link
Contributor

Closes #12872

Summary

  • Adds some missing aira-label attributes to the Edit and Clear buttons on the Advanced Settings page.
  • Adds a getAriaName function and tests that formats the setting names for the labels.

ie: 'search:queryLanguage:switcher:enable' -> 'search query language switcher enable'

* @returns {string} a space demimited, lowercase string with
* special characters removed.
*
* Example: 'xPack:fooBar:foo_bar_baz' -> 'x pack foo bar foo bar baz'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a list somewhere of all the possible advanced configs? I'm just hoping to verify this logic will hold true for all possible values

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, maybe we could make a text file or read it from there and utilize that complete list in the unit test. Otherwise this looks great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a master list. However, if there are specific cases you think we should test for, I can add additional tests. Seems overkill to process / maintain the entire list, when the logic really boils down to how it handles certain characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair!

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably okay since we're not relying on a certain delimiter and I'm guessing/hoping the lodash words method is robust enough to handle any future settings

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -166,6 +168,7 @@
ng-if="!conf.editing"
ng-click="clear(conf)"
ng-hide="isDefaultValue(conf)"
aria-label="Clear {{conf.ariaName}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would Reset {{conf.ariaName}} to default maybe be more descriptive to users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dunno. The button is labelled 'clear' so that's what I used for reference for the aria-label.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with whatever you chose :)

@BigFunger
Copy link
Contributor Author

6.x/6.1: 13d9a85
6.0: 030e5e2

@BigFunger BigFunger deleted the advanced-settings-accessibility branch August 16, 2017 15:43
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