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

Changed Personal and Company Address Labels #4636

Merged
merged 14 commits into from
Aug 17, 2021

Conversation

Luke9389
Copy link
Contributor

@Luke9389 Luke9389 commented Aug 13, 2021

Details

Fixed Issues

$ #4631

Tests / Web QA

  1. Head over to http://localhost:8080/bank-account/new or http://staging.new.expensify.com/bank-account/new
  2. Click Connect Manually
  3. Enter 123123123 for both fields
  4. Observe the label on the Company Address input.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-08-16 at 11 33 21 AM

Mobile Web

Desktop

iOS

Android

cc @MitchExpensify

@Luke9389 Luke9389 self-assigned this Aug 13, 2021
@Luke9389 Luke9389 requested a review from a team as a code owner August 13, 2021 00:04
@MelvinBot MelvinBot requested review from tylerkaraszewski and removed request for a team August 13, 2021 00:04
@Luke9389 Luke9389 requested a review from Jag96 August 13, 2021 00:06
@Luke9389 Luke9389 marked this pull request as draft August 13, 2021 00:06
@Luke9389 Luke9389 marked this pull request as ready for review August 13, 2021 00:13
@Luke9389
Copy link
Contributor Author

I'm assuming @shawnborton will want a little bit more space between the input and the text, but anything else Shawn?

Copy link
Contributor

@Jag96 Jag96 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, just left the 1 comment for the issue caught by the linter. Will let design have a look before testing.

// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(field, ['label', 'fieldName'])}
/>
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the linter caught this, but you can just use <> instead for this

@shawnborton
Copy link
Contributor

Can we use our hint text styles for the text that goes under the input? It should be at 13pt (in our fontSizeLabel var), in our colorMuted text color, and have a margin between the label and the input of 8px (or mt2). We might already have a style for it.

@MitchExpensify
Copy link
Contributor

+1 to @shawnborton

@Luke9389
Copy link
Contributor Author

Updated the code and the screenshot in the description. Lemme know what you think @shawnborton
I wouldn't mind scooting the message to the right but it depends on whether that's consistent with our design philosophy.

@Luke9389 Luke9389 requested a review from Jag96 August 13, 2021 21:24
@shawnborton
Copy link
Contributor

Looks good! I like it aligned to the left, I think that is consistent with other places as well. The only thing I am curious about is perhaps we want to make the gap between input and hint just 4px, this way it looks more like the hint is tied to the input above it? Otherwise right now it kinda floats perfectly in between inputs and doesn't look quite visually connected to what's above. Thoughts on that?

@Luke9389
Copy link
Contributor Author

Yea I think it looks better with 4px 👍
Screen Shot 2021-08-16 at 11 33 21 AM

@shawnborton
Copy link
Contributor

Looks good to me, thanks Luke! All yours @Jag96 and @tylerkaraszewski

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM

@Jag96 Jag96 merged commit a9814af into main Aug 17, 2021
@Jag96 Jag96 deleted the luke-change-company-and-personal-address-labels branch August 17, 2021 14:18
@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.

@Jag96
Copy link
Contributor

Jag96 commented Aug 17, 2021

Going to CP this to staging to check it off the deploy checklist

github-actions bot pushed a commit that referenced this pull request Aug 17, 2021
…nal-address-labels

Changed Personal and Company Address Labels

(cherry picked from commit a9814af)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @Jag96 in version: 1.0.85-9 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Jag96 in version: 1.0.85-10 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants