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

Card Screen - p character on 'Help' button is cropped #4866

Closed
isagoico opened this issue Aug 27, 2021 · 23 comments
Closed

Card Screen - p character on 'Help' button is cropped #4866

isagoico opened this issue Aug 27, 2021 · 23 comments
Assignees
Labels
Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@isagoico
Copy link

isagoico commented Aug 27, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Issue found while executing #4637

Action Performed:

  1. Open app and login
  2. Go to FAB > New Workspace
  3. Tap on Company Cards

Expected Result:

User is able to see guide call button (Help button) with no issues

Actual Result:

p character is cropped

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.0.88-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Expensify/Expensify Issue URL:

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @iwiznia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@isagoico
Copy link
Author

isagoico commented Aug 27, 2021

@alex-mechler This was found while executing QA for #4637. I wasn't sure on adding the deploy blocker label here since this feature is not available in prod yet (and it's a small visual issue imo). Could you review this?

@alex-mechler
Copy link
Contributor

alex-mechler commented Aug 27, 2021

@isagoico I agree, I don't think this is a deploy blocker for the reasons you mentioned! I do think we should fix this though. What device or screensize did this happen on? Will take this over from Ioni

@alex-mechler alex-mechler assigned alex-mechler and unassigned iwiznia Aug 27, 2021
@isagoico
Copy link
Author

isagoico commented Aug 27, 2021

I was able to reproduce this both on an iPhone 6S for iOS and a Huawei P20 Pro (6.2-inch screen size) for Android

@alex-mechler
Copy link
Contributor

Awesome, I'll look into this! Thanks!

@alex-mechler
Copy link
Contributor

Ahhhh, I figured out why this is happening. I was using a small button, but not small text. With small text, it looks fine on mobile.
image

However, it looks a bit off to me on web
image

@Expensify/design any suggestions on what we can try here?

@shawnborton
Copy link
Contributor

Small buttons should be using smaller text by default right? Basically if you pass in the "small" prop to the button, I would assume it would use the small font size (11px) by default.

In terms of this looking a bit off... maybe it's just the icon that feels a bit off? So perhaps for small buttons, if the icons were a bit smaller (like 16x16 instead of 20x20?) they might look better?

@isagoico
Copy link
Author

Issue reproducible during KI retests

@alex-mechler
Copy link
Contributor

Small buttons should be using smaller text by default right? Basically if you pass in the "small" prop to the button, I would assume it would use the small font size (11px) by default.

Ahh, this is because a normal button did not work in this case, I had to create my own "button" with similar styles to get the icon in there.

In terms of this looking a bit off... maybe it's just the icon that feels a bit off? So perhaps for small buttons, if the icons were a bit smaller (like 16x16 instead of 20x20?) they might look better?

I agree, I think that the icon being large is what is throwing me off, I can try with a smaller icon and will post results here

@MelvinBot MelvinBot removed the Overdue label Aug 30, 2021
@shawnborton
Copy link
Contributor

Is it possible to do this using the same button component though? I think ideally we don't want a different kind of button to maintain.

@alex-mechler
Copy link
Contributor

Possibly, I can look into refactoring that. It might take a minute, so let me test with the icon sizes first, so that we can settle on look before worrying about that.

@alex-mechler
Copy link
Contributor

Hmm, 16x16 is already what "small" is for an icon.
image

How about adding an xsmall of 12x12?
image
image

@shawnborton
Copy link
Contributor

That works for me. So that makes me wonder, where else do we have these small buttons that have icons... maybe nowhere?

@shawnborton
Copy link
Contributor

What I am saying is, I wonder if 12x12 should just be the default for small buttons instead of having to add an XS class.

@alex-mechler
Copy link
Contributor

That works for me. So that makes me wonder, where else do we have these small buttons that have icons... maybe nowhere?

The button component currently has no way to show an icon as well, so I think this is the only case where we do.

What I am saying is, I wonder if 12x12 should just be the default for small buttons instead of having to add an XS class.

I would default the text to be textSmall and iconXSmall when the button is small.

@michelle-thompson
Copy link
Contributor

Unpopular opinion here, but I actually feel like 12x12 is too small. I think it's hard to distinguish on web what that icon is. Plus, it looks so much smaller than the grey icons that it feels a little distracting.

@shawnborton
Copy link
Contributor

Fair point Michelle, and maybe the phone icon itself is a weird one.. maybe a more common case of having a down/up caret wouldn't look so strange.

Okay, so maybe we just keep the icon at 16x16 then and make sure the font size is 11pt (like other small buttons) and call it a day?

@alex-mechler
Copy link
Contributor

Works for me, I can also do that refactor to make this not a special case button

@shawnborton
Copy link
Contributor

Sounds great to me - hbu Michelle?

@michelle-thompson
Copy link
Contributor

Sounds great, thanks guys!

@alex-mechler alex-mechler added the Reviewing Has a PR in review label Aug 30, 2021
@botify botify closed this as completed Aug 31, 2021
@botify
Copy link

botify commented Aug 31, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@botify
Copy link

botify commented Sep 1, 2021

🚀 Deployed to staging by @flodnv in version: 1.0.90-3 🚀

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

@botify
Copy link

botify commented Sep 2, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.91-0 🚀

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
Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants