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

Remove pointer events from label, modify styling of textinput #5858

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

AlfredoAlc
Copy link
Contributor

@AlfredoAlc AlfredoAlc commented Oct 14, 2021

@parasharrajat @marcaaron

Details

To fix the initial issue of the label being able to be selected and to loose focus when clicking on it, I set the prop pointerEvents to none. This allows to remove the zIndex prop, and I also removed the selectable prop since it has no use now.

Pretty sure this caused a regression with labels on autofilled forms. Since they have a z-index of -1 they're now hidden 2021-10-13_13-35-13

The autofill also expands beyond the edges but unsure if that was introduced in this PR.

For the issue where the autofill expands beyond the edges, I don't think it was related with the changes I made since I tested it with the same props that had before and actually the result was this:
autofill style

But I passed the borderRadius from the container to the TextInput and added a height since if no height is set, the blue background overlaps the border from the container. Also removed the ...spacing.pv0 because it has paddings props so it had no use.

Fixed Issues

$ #4515
$ #5704

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Android

android

iOS

ios

Desktop

Desktop

Web

Web

@AlfredoAlc AlfredoAlc requested a review from a team as a code owner October 14, 2021 21:30
@MelvinBot MelvinBot requested review from timszot and removed request for a team October 14, 2021 21:30
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM.

@parasharrajat
Copy link
Member

Please CP staging this. @timszot

@parasharrajat
Copy link
Member

@AlfredoAlc Please attach screenshots of all platforms if even if autofill does not work on all but you can attach normal view.

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 14, 2021

Hold on. I need to check something. @AlfredoAlc Did you test it with autofill on the web? Do attach that screen? Thanks.

@AlfredoAlc
Copy link
Contributor Author

@parasharrajat I couldn't find ExpensiTextInput with autofill, I only found in the Add a Debit card inputs with autofill but it had another type of input, I change those inputs with ExpensiTextInput for testing.

Android

android

iOS

ios

Desktop

Desktop

Web

Web

@parasharrajat
Copy link
Member

Ok. No worries, I just wanted to see the autofill working. Thanks for the screens. You can attach these to the main details so that they are easier to find.

I think they look fine. Previously the autofill color was only filling in the input area but now it's covering the whole input. It does not seem a big deal to me. So 👍 .

All yours @timszot.

@timszot timszot merged commit 1ace10c into Expensify:main Oct 14, 2021
github-actions bot pushed a commit that referenced this pull request Oct 14, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @timszot in version: 1.1.7-25 🚀

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

@mvtglobally
Copy link

@AlfredoAlc @parasharrajat Can you share steps to QA this PR

@parasharrajat
Copy link
Member

You can follow the steps for #5704.
and with respect to that please test the autofill as well.

  1. When value auto-fills, a label should not be hidden and auto-filled color should not overflow the Input.
  2. I am sure where will you get the autofill but the best guess is ComanyStep on VBA flow.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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

@anthony-hull
Copy link
Contributor

You can follow the steps for #5704. and with respect to that please test the autofill as well.

  1. When value auto-fills, a label should not be hidden and auto-filled color should not overflow the Input.
  2. I am sure where will you get the autofill but the best guess is ComanyStep on VBA flow.

with 1Password and a card saved, on the add payment method screen you can add a debit card. letting 1password fill this in triggers the blue background

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.

6 participants