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

fix: (platform) input group states #3333

Merged
merged 4 commits into from
Sep 21, 2020

Conversation

dimamarksman
Copy link
Contributor

Please provide a link to the associated issue.

#3324
#3316

Please provide a brief summary of this pull request.

Added disabled state for fdp-input-group input and addon buttons.
In example added tootlip for an icon.

Please check whether the PR fulfills the following requirements

Documentation checklist:

@dimamarksman dimamarksman changed the title Fix platform input group states Fix: (platform) input group states Sep 18, 2020
@dimamarksman dimamarksman changed the title Fix: (platform) input group states fix: (platform) input group states Sep 18, 2020
@dimamarksman dimamarksman changed the title fix: (platform) input group states fix(platform): input group states Sep 18, 2020
@netlify
Copy link

netlify bot commented Sep 18, 2020

Deploy preview for fundamental-ngx ready!

Built with commit 25d00e2

https://deploy-preview-3333--fundamental-ngx.netlify.app

Copy link
Member

@KevinOkamoto KevinOkamoto left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -30,7 +30,7 @@
<p>Icon as an addon</p>
<fdp-input-group name="employee" placeholder="Email">
<fdp-input-group-addon>
<i class="sap-icon--email"></i>
<span class="sap-icon--email" title="Email"></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change it to a span? Also, you probably need aria-hidden="true" for a11y purposes. You can check some blog posts about it https://a11y-101.com/development/icons-and-links

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also in the docs part so you can ignore my comment :) Just wanted to say it's a good to have thing :)

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 added aria-hidden and reverted <span> to <i> (chnaged to span previously because it makes icon in italic, but that's should be fixes once latest fundamental-styles gets released).
Thanks!

@InnaAtanasova InnaAtanasova changed the title fix(platform): input group states fix: (platform) input group states Sep 18, 2020
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.

None yet

4 participants