-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(autocomplete): adds autocomplete text-field #1418
Conversation
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
@marcysutton Can you please confirm that I did not break anything with my re-organization of the aria code? |
Oh my... cannot wait to review this code. #woot |
I maybe jumped the gun on doing a PR - there are no unit tests yet, but I wanted to get this up for review early. Also missing: Documentation and Theming. |
@@ -148,6 +148,18 @@ input { | |||
} | |||
} | |||
|
|||
.visuallyhidden { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use a dash to be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is a common utility from HTML5 Boilerplate, so by not using the hyphen it will be easier to remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps something like .hide-visually
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated (and also removed redundant class in doc styles)
@robertmesserle - is this ready to merge ? |
I have to add documentation and unit tests - should have that in tomorrow.
|
90d9daa
to
8bf5295
Compare
CLAs look good, thanks! |
8bf5295
to
62cb0ff
Compare
@ThomasBurleson Ready to merge. |
height: 40px; | ||
} | ||
} | ||
md-content { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be global?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed in latest commit
@robertmesserle sorry for the late feedback. Nothing major, just a few little things! |
@marcysutton No worries, addressed your feedback and update PR |
ef60d49
to
f9948c6
Compare
feat(autocomplete): adds accessibility support TODO: wire aria-activedescendant as a watched property with a value of the active listItem id refactor(autocomplete): re-organizes aria changes to live in controller chore(autocomplete): removes temporary comments refactor(autocomplete): renames ambiguous directive refactor(styles): renames `visuallyhidden` to `visually-hidden` for consistency. refactor(autocomplete): removes unused template file refactor(autocomplete): uses `$mdConstant` rather than hard-coded values refactor(autocomplete): various cleanup based on feedback refactor(autocomplete): cleans up scope confusion refactor(autocomplete): includes theming, updates directive name to prevent potential conflicts
No description provided.