Text & Dropdown Input Styles

[Luke9389]

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/12445 $ https://github.com/Expensify/App/issues/13741

Tests

• [ ] Verify that no errors appear in the JS console

What to do:

Verify that the inputs and dropdowns look like the following:	Focused	Unfocused	Error	Dropdown

Additionally, be sure to check for off-looking background colors.	❌	✅

Where to do it

1. Every settings page
2. FAB ->
   • New Chat Search Input
   • New Group Search Input
   • New Room Search Input
   • Send Money Search Input
   • Request Money Search Input
   • Split Bill Search Input
3. Search Bar at the top of the LHN

QA Steps

• [x] Verify that no errors appear in the JS console

Same as above tests

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android / native
  • [x] Android / Chrome
  • [x] iOS / native
  • [x] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [x] All JSX used for rendering exists in the render method
  • [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [x] If any new file was added I verified that:
  • [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

- [x] I linked the correct issue in the `### Fixed Issues` section above - [x] I wrote clear testing steps that cover the changes made in this PR - [x] I added steps for local testing in the `Tests` section - [x] I added steps for the expected offline behavior in the `Offline steps` section - [x] I added steps for Staging and/or Production testing in the `QA steps` section - [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct) - [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline) - [x] I tested this PR with a [High Traffic account](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts) against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability). - [x] I included screenshots or videos for tests on [all platforms](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#make-sure-you-can-test-on-all-platforms) - [x] I ran the tests on **all platforms** & verified they passed on: - [x] Android / native - [x] Android / Chrome - [x] iOS / native - [x] iOS / Safari - [x] MacOS / Chrome / Safari - [x] MacOS / Desktop - [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed) - [x] I followed proper code patterns (see [Reviewing the code](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md#reviewing-the-code)) - [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. `toggleReport` and not `onIconClick`) - [x] I verified that comments were added to code that is not self explanatory - [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing. - [x] I verified any copy / text shown in the product is localized by adding it to `src/languages/*` files and using the [translation method](https://github.com/Expensify/App/blob/4bd99402cebdf4d7394e0d1f260879ea238197eb/src/components/withLocalize.js#L60) - [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the [localization methods](https://github.com/Expensify/App/blob/4bd99402cebdf4d7394e0d1f260879ea238197eb/src/components/withLocalize.js#L60-L68) - [x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the `Waiting for Copy` label for a copy review on the original GH to get the correct copy. - [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README. - [x] I verified the JSDocs style guidelines (in [`STYLE.md`](https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#jsdocs)) were followed - [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers - [x] I followed the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md) - [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like `Avatar`, I verified the components using `Avatar` are working as expected) - [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests) - [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such - [x] I verified that if a function's arguments changed that all usages have also been updated correctly - [x] If a new component is created I verified that: - [x] A similar component doesn't exist in the codebase - [x] All props are defined accurately and each prop has a `/** comment above it */` - [x] The file is named correctly - [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone - [x] The only data being stored in the state is data necessary for rendering and nothing else - [x] For Class Components, any internal methods passed to components event handlers are bound to `this` properly so there are no scoping issues (i.e. for `onClick={this.submit}` the method `this.submit` should be bound to `this` in the constructor) - [x] Any internal methods bound to `this` are necessary to be bound (i.e. avoid `this.submit = this.submit.bind(this);` if `this.submit` is never passed to a component event handler like `onClick`) - [x] All JSX used for rendering exists in the render method - [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions - [x] If any new file was added I verified that: - [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory - [x] If a new CSS style is added I verified that: - [x] A similar style doesn't already exist - [x] The style can't be created with an existing [StyleUtils](https://github.com/Expensify/App/blob/main/src/styles/StyleUtils.js) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG`) - [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like `Avatar` is modified, I verified that `Avatar` is working as expected in all cases) - [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected. - [x] If a new page is added, I verified it's using the `ScrollView` component to make it scrollable when more elements are added to the page. - [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

[Luke9389]

This PR is held on the color swapping PR (so we can fully test it in its completed form).

[shawnborton]

Are we planning to wait until the color changes are finished before merging this? Otherwise I would say this could be merged first since all it does is change shapes and styles and not colors.

[Luke9389]

@shawnborton, when testing this I came across this difference that gave me pause. Let me know what you think.

	Old	New
Zoomed In
In Context

Does the text on the new one look a little scrunched to the left to you?

Here it is with no background color, but then it gets hard to distinguish that from the active/clickable ones.

[shawnborton]

Personally I think it looks great!

On Mon, Dec 5, 2022 at 7:31 PM Luke Donahue @.***> wrote:

@shawnborton https://github.com/shawnborton, when testing this I came across this difference that gave me pause. Let me know what you think. Old New Zoomed In [image: Screen Shot 2022-12-05 at 7 27 16 PM] https://user-images.githubusercontent.com/49007721/205805881-551193ea-a33d-47bc-bfee-917f18d42d02.png [image: Screen Shot 2022-12-05 at 7 27 07 PM] https://user-images.githubusercontent.com/49007721/205805903-30ed6433-4d36-4480-a8bf-aa4a29fdaab6.png In Context [image: Screen Shot 2022-12-05 at 7 29 22 PM] https://user-images.githubusercontent.com/49007721/205806107-31c7cbab-5ef3-43cd-9a3d-762052e3f4c9.png [image: Screen Shot 2022-12-05 at 7 29 51 PM] https://user-images.githubusercontent.com/49007721/205806125-8bf1ef20-9322-4c0d-bf28-14b9539ffc32.png

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/pull/12447#issuecomment-1338692293, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARWH5XSIMVELA3CJ5S2E53WL2XPZANCNFSM6AAAAAARWP2LXA . You are receiving this because you were mentioned.Message ID: @.***>

[Luke9389]

Awesome! That's definitely the path of least effort for me! 😄

[melvin-bot[bot]]

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

[Luke9389]

K this one is ready for review!

[grgia]

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

• [x] I have verified the author checklist is complete (all boxes are checked off).
• [x] I verified the correct issue is linked in the ### Fixed Issues section above
• [x] I verified testing steps are clear and they cover the changes made in this PR
  • [x] I verified the steps for local testing are in the Tests section
  • [x] I verified the steps for Staging and/or Production testing are in the QA steps section
  • [x] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• [x] I checked that screenshots or videos are included for tests on all platforms
• [x] I included screenshots or videos for tests on all platforms
• [x] I verified tests pass on all platforms & I tested again on:
  • [x] iOS / native
  • [x] Android / native
  • [x] iOS / Safari
  • [x] Android / Chrome
  • [x] MacOS / Chrome
  • [x] MacOS / Desktop
• [x] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• [x] I verified proper code patterns were followed (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product was added in all src/languages/* files
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I verified that this PR follows the guidelines as stated in the Review Guidelines
• [x] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [x] All JSX used for rendering exists in the render method
  • [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

https://user-images.githubusercontent.com/38015950/206052398-74b4b68c-5aa7-40cc-a1a5-2e44325ec017.mov

Mobile Web - Chrome

https://user-images.githubusercontent.com/38015950/206056715-4702b46c-6534-4914-a915-0c825a2c4cea.mov

Mobile Web - Safari

https://user-images.githubusercontent.com/38015950/206056476-8554497e-f8bd-494b-9dee-1e1d5b7e3f34.mov

Desktop

https://user-images.githubusercontent.com/38015950/206053039-4cd91bd5-32ae-4645-b38f-499fa59a9fce.mov

iOS

Android

[grgia]

I noticed that on focus the text is being cut off: (IOS)

[Luke9389]

Oh nice catch!

[Luke9389]

Should be fixed now:

[grgia]

Testing again!

[grgia]

One more small thing- I noticed that the checkbox focus color is wrong, it should be a green border on clicked.

your branch:

https://user-images.githubusercontent.com/38015950/206053669-1bf4ad19-9696-43fa-98db-ec308cf7fae9.mov

staging:

https://user-images.githubusercontent.com/38015950/206053835-fc1f1b84-d99f-41cf-8371-4797b5ac2bea.mov

[grgia]

It looks like it's occurring on main too actually

[Luke9389]

There's an open PR for this here I think: https://github.com/Expensify/App/pull/13181

[Luke9389]

🤷 Idk, but I think that's out of scope for this PR. We doin text inputs and dropdowns.

[shawnborton]

How did you fix the bottom portion of the text from being cut off? Maybe you need to push your code? I don't see anything that explains how it was fixed so I'm just curious, thanks!

[shawnborton]

On Android it looks like the input value is not lined up with the left edge like the label is:

Also, are we planning to have a C+ do a thorough review/test of this too?

[Luke9389]

@shawnborton I think you may need to pull my branch. Both of those last comments have been addressed.

Also, are we planning to have a C+ do a thorough review/test of this too?

I wasn't planning on it. I've been testing thoroughly on each platform

[shawnborton]

I think we should have a C+ do a thorough review. From the previous PRs for the branding changes, we've had to do quite a few follow up PRs for little design bugs we didn't catch, so I would prefer to have the extra set of eyes here.

[Luke9389]

OK! Thats fine w/ me.

I'll apply External on the parent issue and explain it's for a review.

[parasharrajat]

Because we removed the left padding, should we also adjust the arrow sign on select/picker? IMO, we should reduce the right padding from the arrow. @shawnborton

[shawnborton]

I think this might be a result of removing the line height? But 52px is what we had previously, so ideally we have the same height with this new change.

[shawnborton]

IMO, we should reduce the right padding from the arrow. @shawnborton

Agree with that

[shawnborton]

Web: Slight dimensions change to the left-sidebar on the login page.

Are we sure that's related to this PR?

[parasharrajat]

https://github.com/Expensify/App/pull/12447#issuecomment-1341516208

Will check on it?

[parasharrajat]

Bump @Luke9389

[Luke9389]

Hey y'all. Thanks for the reviews. Just getting back around to this one. I'll be addressing all the comments and bugs, and then hopefully we can wrap this up.

[Luke9389]

BUG: Height of textInput and picker are not the same.

Is this actually a problem? This wasn't on my radar as a constraint that needed to be met.

[shawnborton]

I agree that it is a problem - the inputs should all appear to be the same height no matter if its a text input or a select menu or a date picker. Furthermore, the label and input text should also appear to be identical between all of them, too:

[Luke9389]

It looks like the height text inputs and dropdowns are also unequal on staging and prod. Which height should we use? 50 or 52? I'm leaning toward 52 because it's a multiple of 4, which we tend to like, right?

[shawnborton]

Yup, they should be 52. Thanks!

[Luke9389]

There was a variable set here that I've just changed: https://github.com/Expensify/App/blob/07d6fdbafefd24acf400e4123b3922cdcc891d22/src/styles/variables.js#L77

Looks good in the dev environment but as we know that's not always sufficient these days.

[shawnborton]

Wow that is strange, I wonder what the context is there for why it exists in the first place? Maybe you can search the commit history behind it?

[Luke9389]

Looks like it was introduced in this PR, which seems to be adjusting height values to account for device font sizes.

Here's the issue it was designed to solve: https://github.com/Expensify/App/issues/7541

[Luke9389]

OK @parasharrajat, I've addressed all the reviews you've left above, with the exception of the layout change between staging and this branch.

@shawnborton, what do you think we should do about that one? I could add some styles in here to make them look the same. Before doing that I just want to see if you think it's a good idea to make those changes here.

[Luke9389]

Either way, would you be willing to test on all 6 again @parasharrajat ?

[shawnborton]

layout change between staging and this branch.

What exactly do you mean by that? Can you share screenshots of this?

[Luke9389]

Sure! It was this thing that @parasharrajat mentioned: https://github.com/Expensify/App/pull/12447#pullrequestreview-1207855990

[shawnborton]

Ah interesting. Any idea why that is happening?

[Luke9389]

OH heh, I think my chrome zoom level was off!

They look the same when both displayed at 100%.

[Luke9389]

@parasharrajat is that the case for you too?

stg	dev

[shawnborton]

Also @Luke9389 I wanted to confirm that we fixed the text clipping issue that was happening on iOS with inputs? We can test that by using words that have a lowercase g or even using emoji in the input. Thanks!

[parasharrajat]

I will test it again. Wasn't working yesterday.

[Luke9389]

I wanted to confirm that we fixed the text clipping issue that was happening on iOS with inputs

@shawnborton Yep, this has been fixed here:

[Luke9389]

I'm noticing that the placeholder text looks a little high now on Last name above. Do you agree @shawnborton?

I think this came along with our adjustment to make the text inputs slightly taller.

[Luke9389]

Also I fixed the issue reported here.

before	after

[Luke9389]

Also I've just tested these changes against he bug report here, and it works 🎉 (This is with the iPhone SE, which was the device that was clipping the most in that bug report)

[shawnborton]

I think the placeholder looks good to me, but perhaps one way to quickly test is to give the input a background color so we can see if the placeholder is vertically centered in that 52px piece of UI.

[shawnborton]

Tested and so far this is looking great!

Super nitpicky, but I am noticing that in the workspace card for mileage, the input and select menu aren't perfectly aligned:

And I think that's largely because the select menu is at 54px tall instead of 52px: