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

Add counter and hint to TextInput #7577

Merged
merged 10 commits into from
Feb 11, 2022

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Feb 4, 2022

Details

Fixed Issues

$ #7522

Tests | QA Steps

  1. Check that there is no visual change to the TextInput itself.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

image
image

Mobile Web

image

iOS

image

Android

image

@parasharrajat parasharrajat requested a review from a team as a code owner February 4, 2022 21:33
@MelvinBot MelvinBot requested review from puneetlath and removed request for a team February 4, 2022 21:33
@puneetlath
Copy link
Contributor

Can you help me understand why you chose to make FormHelp a separate component. Is it because we plan to use the same form help text on other input types?

@parasharrajat
Copy link
Member Author

Can you help me understand why you chose to make FormHelp a separate component. Is it because we plan to use the same form help text on other input types?

Yeah, exactly. I think other components(picker, DatePicker, etc) will need the same to apply the correct margin and padding. So we will eventually give up InlineErrorText component.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Left a few comments

src/styles/styles.js Outdated Show resolved Hide resolved
src/components/TextInput/baseTextInputPropTypes.js Outdated Show resolved Hide resolved
src/components/FormHelp.js Outdated Show resolved Hide resolved
@luacmartins
Copy link
Contributor

Yeah, exactly. I think other components(picker, DatePicker, etc) will need the same to apply the correct margin and padding. So we will eventually give up InlineErrorText component.

I'm not sure we will end up reusing this. Those inputs will most likely not require a hint nor character count.

@parasharrajat
Copy link
Member Author

Changes done.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Left a small comment. Can we also add a screenshot for iOS as well?

src/components/TextInput/BaseTextInput.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

OK, Changes are done @luacmartins @puneetlath. Updated screenshots. Thanks.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

LGTM and tested well! Thanks for the changes @parasharrajat! All yours @puneetlath.

Copy link
Contributor

@puneetlath puneetlath 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 to me too!

@luacmartins luacmartins merged commit 0944de1 into Expensify:main Feb 11, 2022
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @luacmartins in version: 1.1.39-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Stutikuls
Copy link

Stutikuls commented Mar 7, 2022

Issue 1 -

Title- [Medium]: Chrome+ Jaws : Screen reader :Role is not defined for the 'Save' control.
Actual - Focus lands on the Save control and screen reader is reading "Save".
Note - Reading "Group" on MAC.
Expected - Role = 'Button' should be defined for the Save control and when focus lands on the Save control then screen reader should read "Save button".
WCAG failure - 4.1.2
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Mobile-Web, iOS

7577_Role.is.not.defined.for.Save.control.mp4

Issue 2 -

Title-[High] Chrome + Jaws: Screen reader : Error message is not announced by the screen reader
Actual - Screen reader does not announce any inline error message appear below to text boxes.
Expected - Inline error message should be conveyed to the users as soon as it appear on the fields so that user can resolve the error.
WCAG failure - 3.3.1
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Web, Desktop, Mobile-web, iOS, Android

7577_Error.message.is.not.read.by.screen.reader.mp4

Issue 3 -

Title-[Medium] Chrome +Jaws: Screen reader : Status message is not being announced by screen reader
Actual - Status message 'Your profile was successfully saved' is not announced by the screen reader.
Expected - Status message 'Your profile was successfully saved' should announce by the screen reader.
WCAG failure - 4.3.1
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Web, Desktop(Voiceover), Mobile-web(Safari + Voiceover), iOS. Android

7577_Status.message.not.read.by.screen.reader.mp4

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.

5 participants