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

feat(text-input): add warn prop #5918

Merged
merged 16 commits into from
Jul 20, 2020

Conversation

janhassel
Copy link
Member

@janhassel janhassel commented Apr 23, 2020

Related issue: #6492

This is a draft PR with a sample implementation of a warning state for the TextInput component to start a conversation around the technical side of things.

@aagonzales Right now, the icon is still the round warning icon. I haven't been able to change it yet, since the triangle one doesn't have a separate inner path to color the exclamation mark. I'll open an issue for it shortly.

@joshblack I'd love your input about the implementation and if this would be the right way to go. Also, for accessibility: I believe aria-invalid shouldn't be emitted since it could lead to confusion for users that rely on screen readers, would you agree?

@netlify
Copy link

netlify bot commented Apr 23, 2020

Deploy preview for carbon-elements ready!

Built with commit 1d52468

https://deploy-preview-5918--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 23, 2020

Deploy preview for carbon-components-react ready!

Built with commit 1d52468

https://deploy-preview-5918--carbon-components-react.netlify.app

@aagonzales
Copy link
Member

The warning text color should match the label color and use $text-02. Other than and the using the warning-alt icon when its ready it looks fine from a visual perspective.

Also a good thing to note is we are planning on moving the helper text to be under the input instead of under the label so this will override any helper text that is there as well.

@janhassel
Copy link
Member Author

@aagonzales Thanks for the heads-up. The helper text update will be included in #5867 it seems. So I’ll make sure to update the warning text logic accordingly once that is merged 👍

@joshblack
Copy link
Contributor

I believe aria-invalid shouldn't be emitted since it could lead to confusion for users that rely on screen readers, would you agree?

Makes sense to me!

@janhassel
Copy link
Member Author

I think the only thing blocking this then would be #5919. Once that is fixed, I can update the snapshot and this should be pretty much good to go. Support for the other inputs could then be added one by one in new PRs to keep them small and manageable.

@janhassel janhassel marked this pull request as ready for review July 16, 2020 11:59
@janhassel janhassel requested review from a team as code owners July 16, 2020 11:59
@andreancardona andreancardona self-assigned this Jul 16, 2020
@joshblack
Copy link
Contributor

joshblack commented Jul 16, 2020

@andreancardona @rjhenriquez let me know if you all have questions around this PR / review process!

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

@janhassel looks awesome! 🎉

@tw15egan tw15egan linked an issue Jul 20, 2020 that may be closed by this pull request
@tw15egan tw15egan requested review from a team and aagonzales and removed request for a team July 20, 2020 17:14
@tw15egan
Copy link
Collaborator

Screen Shot 2020-07-20 at 10 14 50 AM

For visual review since Netlify is having problems

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

🎉 Looks awesome!!

@kodiakhq kodiakhq bot merged commit a5e27b7 into carbon-design-system:master Jul 20, 2020
@janhassel janhassel deleted the warning-input branch July 21, 2020 08:10
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.

Contribution: Add yellow warning state to Select
6 participants