[HOLD] Text & Dropdown Input Styles

[Luke9389]

Details

Fixed Issues

$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

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

QA Steps

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

PR Author Checklist

• [ ] I linked the correct issue in the ### Fixed Issues section above
• [ ] I wrote clear testing steps that cover the changes made in this PR
  • [ ] I added steps for local testing in the Tests section
  • [ ] I added steps for Staging and/or Production testing in the QA steps section
  • [ ] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [ ] 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)
• [ ] I included screenshots or videos for tests on all platforms
• [ ] I ran the tests on all platforms & verified they passed on:
  • [ ] iOS / native
  • [ ] Android / native
  • [ ] iOS / Safari
  • [ ] Android / Chrome
  • [ ] MacOS / Chrome
  • [ ] MacOS / Desktop
• [ ] 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)
• [ ] I followed proper code patterns (see Reviewing the code)
  • [ ] 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)
  • [ ] I verified that comments were added to code that is not self explanatory
  • [ ] 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.
  • [ ] I verified any copy / text shown in the product was added in all src/languages/* files
  • [ ] 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.
  • [ ] 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.
  • [ ] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [ ] I followed the guidelines as stated in the Review Guidelines
• [ ] 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)
• [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [ ] 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
• [ ] If a new component is created I verified that:
  • [ ] A similar component doesn't exist in the codebase
  • [ ] All props are defined accurately and each prop has a /** comment above it */
  • [ ] The file is named correctly
  • [ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [ ] The only data being stored in the state is data necessary for rendering and nothing else
  • [ ] 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)
  • [ ] 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)
  • [ ] All JSX used for rendering exists in the render method
  • [ ] 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
• [ ] If a new CSS style is added I verified that:
  • [ ] A similar style doesn't already exist
  • [ ] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [ ] 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)
• [ ] 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.
• [ ] 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

• [ ] I have verified the author checklist is complete (all boxes are checked off).
• [ ] I verified the correct issue is linked in the ### Fixed Issues section above
• [ ] I verified testing steps are clear and they cover the changes made in this PR
  • [ ] I verified the steps for local testing are in the Tests section
  • [ ] I verified the steps for Staging and/or Production testing are in the QA steps section
  • [ ] 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)
  • [ ] 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)
• [ ] I checked that screenshots or videos are included for tests on all platforms
• [ ] I included screenshots or videos for tests on all platforms
• [ ] I verified tests pass on all platforms & I tested again on:
  • [ ] iOS / native
  • [ ] Android / native
  • [ ] iOS / Safari
  • [ ] Android / Chrome
  • [ ] MacOS / Chrome
  • [ ] MacOS / Desktop
• [ ] 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
• [ ] I verified proper code patterns were followed (see Reviewing the code)
  • [ ] 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).
  • [ ] I verified that comments were added to code that is not self explanatory
  • [ ] 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.
  • [ ] I verified any copy / text shown in the product was added in all src/languages/* files
  • [ ] 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.
  • [ ] 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.
  • [ ] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [ ] I verified that this PR follows the guidelines as stated in the Review Guidelines
• [ ] 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)
• [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [ ] 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
• [ ] If a new component is created I verified that:
  • [ ] A similar component doesn't exist in the codebase
  • [ ] All props are defined accurately and each prop has a /** comment above it */
  • [ ] The file is named correctly
  • [ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [ ] The only data being stored in the state is data necessary for rendering and nothing else
  • [ ] 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)
  • [ ] 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)
  • [ ] All JSX used for rendering exists in the render method
  • [ ] 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
• [ ] If a new CSS style is added I verified that:
  • [ ] A similar style doesn't already exist
  • [ ] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [ ] 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)
• [ ] 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.
• [ ] I have checked off every checkbox in the PR reviewer 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: