Fix: input alignment & error border for input #5805

parasharrajat · 2021-10-13T14:58:45Z

Details

Follow up #5742.

I have removed the manual translation from the label on ExpensiInput. Previously, It was required to pass custom translateX prop based on the label length. It was hectic and error-prone.

Fixed Issues

$ #5462

Tests | QA Steps

Label and value alignment should be correct for input and Select input.
When a field has an error it should show a red border.

Tested On

Screenshots

Web | Mobile Web | Desktop

iOS

Android

parasharrajat · 2021-10-13T14:59:46Z

cc @shawnborton. I will update the screens shortly.

src/components/Picker/PickerPropTypes.js

aldo-expensify

The "focus" border can apply at the same time as the red outline, which looks weird. I think it should be just one border:

parasharrajat · 2021-10-13T16:43:56Z

Strange, I did tested that. Let me check again.

aldo-expensify · 2021-10-13T16:48:18Z

Strange, I did tested that. Let me check again.

My guess is that you may still be applying it to the outer container. The "focus" border looks extra thick for the ExpensiPicker

meanwhile, for other inputs, it looks thinner:

parasharrajat · 2021-10-13T17:08:24Z

Done.

aldo-expensify · 2021-10-13T17:36:07Z

Done.

Tested it again in web/android, it is looking good.

Is this still WIP?

shawnborton · 2021-10-13T17:49:00Z

I think we want to keep the :focus styles for accessibility purposes right?

shawnborton · 2021-10-13T17:50:51Z

But that being said, maybe we want the :focus style on an input with an error border to look different? It seems like a pretty standard :focus outline is to take the :focus color, reduce the opacity, and apply it as a thicker drop shadow (with no blur) around the input.. GH for example:

shawnborton · 2021-10-13T17:53:35Z

But actually if we just have a better :focus style for inputs, we don't need to worry about making it match the error red border... another example from GH:

parasharrajat · 2021-10-13T18:25:19Z

Sorry didn't notice the title. I was waiting for Shawn's review.

parasharrajat · 2021-10-13T19:46:52Z

@shawnborton What do you want me to change here? I didn't get your comments clearly.

shawnborton · 2021-10-13T23:32:30Z

@parasharrajat in this screenshot here:

It feels like the "He/him" is over to the right by 1px too far. Can you inspect that one for me and show me the bounding boxes?

aldo-expensify · 2021-10-13T23:33:18Z

But actually if we just have a better :focus style for inputs, we don't need to worry about making it match the error red border... another example from GH: <image>

I agree on that following those styles for focused inputs and errors is the right path, but this may be out of the scope of the original GH issue and the regression this is PR is trying to fix.

shawnborton · 2021-10-13T23:34:50Z

Fair point @aldo-expensify - let's save these for a separate issue.

@parasharrajat just see my comment above... the first screenshot feels a bit off, but the second screenshot you posted in the first comment seems okay.

parasharrajat · 2021-10-14T09:24:07Z

@shawnborton I noticed it. Let me check.

shawnborton · 2021-10-14T14:32:45Z

src/styles/styles.js

        paddingBottom: 8,
-        paddingHorizontal: 11.5,
+        paddingHorizontal: 12,


I thought we discussed changing this to 11?

Oh, I forget that point while figuring out other issues. I will update it.

parasharrajat · 2021-11-29T17:59:07Z

So there won't be any alignment issues in the future. All inputs are now calculating their own label placement position so I am sure it would be 99.999% perfect.
For outline issues that Aldo mentioned

Fix: input alignment & error border for input #5805 (review) Fixed! only red is shown now if the input has an error.
Fix: input alignment & error border for input #5805 (comment) Fixed! Previously both gray and red were shown together now only one is shown (blue, red, or gray) based on the input state.

Please let me know if there is something remaining. This PR thread has so many comments I could have missed some points. But I think fixed all along the way.

I didn't expect that this issue would take this much time as I didn't think the original input implementation has issues and used the wrong props (use of translate props as it is). Now I decided to refactor it.

shawnborton · 2021-11-29T18:23:15Z

Sounds good to me!

parasharrajat · 2021-11-29T18:28:42Z

Maria is in a different time zone. Please give it 🟢 🚦 for merge.

shawnborton · 2021-11-29T18:32:53Z

I think it's okay if Maria merges this tomorrow once she sees it and gets a chance to review. Thanks for your patience!

parasharrajat · 2021-11-30T10:47:20Z

@MariaHCD Please review it.

parasharrajat · 2021-11-30T18:53:50Z

I don't think Maria is available. I see no other reason for holding this. Just not looking for more merge conflicts. cc: @aldo-expensify

shawnborton · 2021-11-30T18:55:25Z

Yup - looks like Maria is OOO this week, so I think we can go ahead and merge.

parasharrajat · 2021-11-30T19:29:26Z

Well...well...well. Look what we got here....conflicts 🤦‍♂️

aldo-expensify · 2021-11-30T19:32:58Z

Well...well...well. Look what we got here....conflicts 🤦‍♂️

:( :( :( :( thanks @parasharrajat , had a laugh with your gif, it was perfect 😆

aldo-expensify · 2021-11-30T19:48:52Z

I guess we wait for the E2E tests and then merge?

aldo-expensify · 2021-11-30T22:39:38Z

Merging, @MariaHCD is OOO this week

OSBotify · 2021-11-30T22:40:31Z

✋ 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 · 2021-12-07T00:21:23Z

🚀 Deployed to staging by @aldo-expensify in version: 1.1.17-8 🚀

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

OSBotify · 2021-12-08T20:13:12Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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

rushatgabhane · 2023-03-21T01:06:41Z

src/styles/styles.js

        color: themeColors.text,
        height: '100%',
-        paddingTop: 25,
+        paddingTop: 23,


Dropping a friendly note that expensiTextInputLabelBackground has height of 25px.

And expensiTextInput's paddingTop was changed to 23px.

This caused issue - #14674 because the paddingTop and height of the label were misaligned.

eVoloshchak · 2023-05-23T17:10:06Z

src/components/ExpensiTextInput/ExpensiTextInputLabel/index.native.js

+                            inputRange: [styleConst.ACTIVE_LABEL_SCALE, styleConst.INACTIVE_LABEL_SCALE],
+                            outputRange: [-(this.state.width - (this.state.width * styleConst.ACTIVE_LABEL_SCALE)) / 2, 0],
+                        }),
+                        this.props.labelScale,


Dropping a friendly note that this has caused a regression in #17715.
There is a detailed explanation here, but in short, on Android, onLayout was firing after the screen animation, setting this.state.width and moving thelabel. This resulted in labels jumping horizontally right after the screen animation on Android

parasharrajat added 2 commits October 13, 2021 19:54

fix: input alignment

9915fab

fix: picker styles

b5bf437

parasharrajat requested a review from a team as a code owner October 13, 2021 14:58

MelvinBot requested review from MariaHCD and removed request for a team October 13, 2021 14:59

parasharrajat changed the title ~~fix: input alignment & error border for input~~ [WIP] Fix: input alignment & error border for input Oct 13, 2021

parasharrajat mentioned this pull request Oct 13, 2021

fix: input styles #5742

Merged

5 tasks

aldo-expensify reviewed Oct 13, 2021

View reviewed changes

src/components/Picker/PickerPropTypes.js Outdated Show resolved Hide resolved

typo

9aef070

aldo-expensify requested changes Oct 13, 2021

View reviewed changes

fix: borders

c45401a

parasharrajat changed the title ~~[WIP] Fix: input alignment & error border for input~~ Fix: input alignment & error border for input Oct 13, 2021

parasharrajat requested a review from aldo-expensify October 13, 2021 19:45

parasharrajat added 2 commits October 14, 2021 15:37

fix: Select value placement

91b46cb

fix: styles

dfab012

shawnborton reviewed Oct 14, 2021

View reviewed changes

shawnborton previously approved these changes Nov 29, 2021

View reviewed changes

Merge branch 'main' of github.com:Expensify/Expensify.cash into ui-fixes

57e9a65

parasharrajat dismissed stale reviews from shawnborton and aldo-expensify via 57e9a65 November 30, 2021 19:40

parasharrajat requested review from aldo-expensify and shawnborton November 30, 2021 19:40

shawnborton approved these changes Nov 30, 2021

View reviewed changes

aldo-expensify approved these changes Nov 30, 2021

View reviewed changes

aldo-expensify merged commit eddcc0f into Expensify:main Nov 30, 2021

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

kavimuru mentioned this pull request Dec 7, 2021

Profile - Able to update profile with emojis and symbols as First and Last name #6610

Closed

rushatgabhane mentioned this pull request Mar 21, 2023

[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

Closed

4 tasks

rushatgabhane reviewed Mar 21, 2023

View reviewed changes

eVoloshchak reviewed May 23, 2023

View reviewed changes

eVoloshchak mentioned this pull request May 23, 2023

[HOLD for payment 2023-05-16] [$1000] Android - Workspaces - Inscriptions move when clicking back in the process of connecting Bank account #17715

Closed

6 tasks

parasharrajat deleted the ui-fixes branch November 20, 2023 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: input alignment & error border for input #5805

Fix: input alignment & error border for input #5805

parasharrajat commented Oct 13, 2021 •

edited

Loading

parasharrajat commented Oct 13, 2021

aldo-expensify left a comment

parasharrajat commented Oct 13, 2021

aldo-expensify commented Oct 13, 2021

parasharrajat commented Oct 13, 2021

aldo-expensify commented Oct 13, 2021

shawnborton commented Oct 13, 2021

shawnborton commented Oct 13, 2021

shawnborton commented Oct 13, 2021

parasharrajat commented Oct 13, 2021

parasharrajat commented Oct 13, 2021

shawnborton commented Oct 13, 2021

aldo-expensify commented Oct 13, 2021 •

edited

Loading

shawnborton commented Oct 13, 2021

parasharrajat commented Oct 14, 2021

shawnborton Oct 14, 2021

parasharrajat Oct 14, 2021

parasharrajat commented Nov 29, 2021

shawnborton commented Nov 29, 2021

parasharrajat commented Nov 29, 2021

shawnborton commented Nov 29, 2021

parasharrajat commented Nov 30, 2021

parasharrajat commented Nov 30, 2021

shawnborton commented Nov 30, 2021

parasharrajat commented Nov 30, 2021

aldo-expensify commented Nov 30, 2021

aldo-expensify commented Nov 30, 2021

aldo-expensify commented Nov 30, 2021

OSBotify commented Nov 30, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

rushatgabhane Mar 21, 2023 •

edited

Loading

eVoloshchak May 23, 2023

Fix: input alignment & error border for input #5805

Fix: input alignment & error border for input #5805

Conversation

parasharrajat commented Oct 13, 2021 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Mobile Web | Desktop

iOS

Android

parasharrajat commented Oct 13, 2021

aldo-expensify left a comment

Choose a reason for hiding this comment

parasharrajat commented Oct 13, 2021

aldo-expensify commented Oct 13, 2021

parasharrajat commented Oct 13, 2021

aldo-expensify commented Oct 13, 2021

shawnborton commented Oct 13, 2021

shawnborton commented Oct 13, 2021

shawnborton commented Oct 13, 2021

parasharrajat commented Oct 13, 2021

parasharrajat commented Oct 13, 2021

shawnborton commented Oct 13, 2021

aldo-expensify commented Oct 13, 2021 • edited Loading

shawnborton commented Oct 13, 2021

parasharrajat commented Oct 14, 2021

shawnborton Oct 14, 2021

Choose a reason for hiding this comment

parasharrajat Oct 14, 2021

Choose a reason for hiding this comment

parasharrajat commented Nov 29, 2021

shawnborton commented Nov 29, 2021

parasharrajat commented Nov 29, 2021

shawnborton commented Nov 29, 2021

parasharrajat commented Nov 30, 2021

parasharrajat commented Nov 30, 2021

shawnborton commented Nov 30, 2021

parasharrajat commented Nov 30, 2021

aldo-expensify commented Nov 30, 2021

aldo-expensify commented Nov 30, 2021

aldo-expensify commented Nov 30, 2021

OSBotify commented Nov 30, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

rushatgabhane Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

eVoloshchak May 23, 2023

Choose a reason for hiding this comment

parasharrajat commented Oct 13, 2021 •

edited

Loading

aldo-expensify commented Oct 13, 2021 •

edited

Loading

rushatgabhane Mar 21, 2023 •

edited

Loading