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

[Hold for reporting bonus] Dev - Pay button text is cut off in Report. #12958

Closed
kavimuru opened this issue Nov 23, 2022 · 21 comments
Closed

[Hold for reporting bonus] Dev - Pay button text is cut off in Report. #12958

kavimuru opened this issue Nov 23, 2022 · 21 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Needs Reproduction Reproducible steps needed

Comments

@kavimuru
Copy link

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


Action Performed:

  1. In LHN, choose any Report that has request money.
  2. Notice that the Pay button is cut off text.

Expected Result:

Pay button text should be fully visible.

Actual Result:

Pay button text is cut off .

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.2.30-0
Reproducible in staging?: Need reproduction
Reproducible in production?: Need reproduction
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Screen.Recording.2022-11-23.at.10.13.58.mov

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669173251556449

View all open jobs on GitHub

@kavimuru kavimuru added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Nov 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 23, 2022

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@grgia grgia assigned grgia and unassigned NicMendonca Nov 23, 2022
@grgia grgia added the Internal Requires API changes or must be handled by Expensify staff label Nov 23, 2022
@grgia
Copy link
Contributor

grgia commented Nov 23, 2022

    buttonSmall: {
        borderRadius: variables.buttonBorderRadius,
        height: variables.componentSizeSmall,
        paddingTop: 10,
        paddingHorizontal: 14,
        paddingBottom: 10,
        backgroundColor: themeColors.buttonDefaultBG,
    },

    buttonMedium: {
        borderRadius: variables.buttonBorderRadius,
        height: variables.componentSizeNormal,
        paddingTop: 6,
        paddingRight: 16,
        paddingBottom: 6,
        paddingLeft: 16,
        backgroundColor: themeColors.buttonDefaultBG,
    },

@shawnborton here's our current small/medium button styles. Currently, this bug is only reproducible on mobile (at least IOS, have yet to test android)

#12762 (comment)
Link to discussion/ fix in IOU Preview for medium button

@grgia
Copy link
Contributor

grgia commented Nov 23, 2022

image

@shawnborton
Copy link
Contributor

I think we just have way too much top/bottom padding for the small buttons. What happens when we reduce it to something smaller, like 4?

@grgia
Copy link
Contributor

grgia commented Nov 23, 2022

image

image

Here's with 4

@shawnborton
Copy link
Contributor

Nice, that feels good to me for the gray buttons. I see in the green "Pay" button in the background, it looks weird. Any idea why that button would look different from the others?

@Puneet-here
Copy link
Contributor

Puneet-here commented Nov 23, 2022

paddingBottom and paddingTop should be 6px to keep the pay button correct

@shawnborton
Copy link
Contributor

I feel like the button text should always be vertically centered no matter what the padding is though. Are we able to do that?

@Puneet-here
Copy link
Contributor

Puneet-here commented Nov 23, 2022

Yes, we can. We have to use styles.justifyContentCenter here

style={[styles.buttonSmall, styles.buttonSuccess, styles.mt4]}

We are using TouchableOpacity for this button why not use Button ?

@melvin-bot
Copy link

melvin-bot bot commented Nov 24, 2022

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot
Copy link

melvin-bot bot commented Nov 24, 2022

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@mdneyazahmad
Copy link
Contributor

I was debugging the issue, not this one, and came to this issue via links. I see a PR is already merged. This can be used to add more context of the issue.

Button has a fixed height of 28, and padding top 10, padding bottom 10 and font size is 14.

The issue here is that the empty space for text (28 - 10 - 10) = 8 is left and the font size is 14. Therefore, the text is cut off.

This issue does not exist in web, because of the inconsistency between the devices. They behave differently. That can be observed from the attached video.

Changing padding top and bottom from 10 to 6 (seems fine), or we can remove this hard coded padding value completely. As the container has a display flex, and it has already set justifyContent to center, so the text will be always in the center provided the height is enough for the text.

Screen.Recording.2022-11-24.at.2.30.39.PM.mov
Screen.Recording.2022-11-24.at.2.36.37.PM.mov

@melvin-bot melvin-bot bot added the Overdue label Nov 28, 2022
@grgia
Copy link
Contributor

grgia commented Nov 29, 2022

@mdneyazahmad That's a good point, and definitely something worth implementing if we run into this issue again! That said, I think for now we can close this issue as the padding changes in the fix fit the current branding in Figma.

@grgia grgia closed this as completed Nov 29, 2022
@melvin-bot melvin-bot bot removed the Overdue label Nov 29, 2022
@hungvu193
Copy link
Contributor

Can I have reporting bonus for this issue?

@Santhosh-Sellavel
Copy link
Collaborator

@grgia Reporting bonus seems missed out for @hungvu193!

@hungvu193
Copy link
Contributor

Should we reopen this?

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

Thanks for the bump, I'll reopen and see about payment

@grgia grgia reopened this Dec 7, 2022
@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

@NicMendonca I must have accidentally unassigned you, do you mind helping me out with the reporting bonus for this issue?

@melvin-bot melvin-bot bot added the Overdue label Dec 9, 2022
@NicMendonca NicMendonca changed the title Dev - Pay button text is cut off in Report. [Hold for reporting bonus] Dev - Pay button text is cut off in Report. Dec 12, 2022
@NicMendonca
Copy link
Contributor

@hungvu193 can you please apply to the job here?

@NicMendonca NicMendonca self-assigned this Dec 12, 2022
@hungvu193
Copy link
Contributor

hungvu193 commented Dec 12, 2022

@hungvu193 can you please apply to the job here?

@NicMendonca it's showing as private job, can you please check again?

Update: I've just applied

@NicMendonca
Copy link
Contributor

@hungvu193 paid! Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Needs Reproduction Reproducible steps needed
Projects
None yet
Development

No branches or pull requests

8 participants