Skip to content

Workspace - Incorrect left padding for the button which has a left icon #29967

Closed

Description

@kbecciv

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.3.87.0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Pujan92
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697713080450809

Action Performed:

  1. Open any screen which has a large button with left icon
  2. Go to workspaces -> select any workspace -> Cards
  3. Observe the left icon alignment for the button "Connect bank account"

Expected Result:

Left icon should not be much closer to the left border

Actual Result:

Left icon is closer to the left border

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Android: Native

Screenshot_1697713163

Android: mWeb Chrome

Screenshot_1697717911

iOS: Native

Simulator Screen Shot - iPhone 14 Pro Max - 2023-10-19 at 16 28 43

iOS: mWeb Safari

Simulator Screen Shot - iPhone 14 Pro Max - 2023-10-19 at 17 45 56

MacOS: Chrome / Safari

Screenshot 2023-10-19 at 16 27 41

MacOS: Desktop

Screenshot 2023-10-19 at 17 41 56

View all open jobs on GitHub

Activity

  1. changed the title [-]Bank Account - Incorrect left padding for the button which has a left icon[/-] [+]Workspace - Incorrect left padding for the button which has a left icon[/+] on Oct 19, 2023
  2. yh-0218 commented on Oct 19, 2023

    @yh-0218
    Contributor

    Proposal

    Please re-state the problem that we are trying to solve in this issue.

    Incorrect left padding for the button which has a left icon

    What is the root cause of that problem?

    We don’t have any additional left padding for left-icon of button.

    <View style={[styles.mr1, ...this.props.iconStyles]}>

    What changes do you think we should make in order to solve the problem?

    We can add additional left padding

    <View style={[styles.mr1, this.props.large && styles.ml2, ...this.props.iconStyles]}>
    

    <View style={[styles.mr1, ...this.props.iconStyles]}>

    What alternative solutions did you explore? (Optional)

  3. OSBotify commented on Oct 19, 2023

    @OSBotify
    Contributor

    👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

    1. Identify the pull request that introduced this issue and revert it.
    2. Find someone who can quickly fix the issue.
    3. Fix the issue yourself.
  4. melvin-bot commented on Oct 19, 2023

    @melvin-bot

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

  5. ginsuma commented on Oct 19, 2023

    @ginsuma
    Contributor

    Proposal

    Please re-state the problem that we are trying to solve in this issue.

    Incorrect left padding for the button which has a left icon

    What is the root cause of that problem?

    This is a regression from #29787. It changes the paddingLeft of the large button to 10.

    Screen Shot 2023-10-19 at 7 54 22 PM

    What changes do you think we should make in order to solve the problem?

    We should add a condition to set paddingLeft is 10 if the button doesn't have an icon to fix #29759, and 18 if yes.

    buttonLarge: (hasIcon: Boolean) => ({
        borderRadius: variables.buttonBorderRadius,
        minHeight: variables.componentSizeLarge,
        paddingTop: 8,
        paddingRight: 10,
        paddingBottom: 8,
        paddingLeft: hasIcon ? 18 : 10,
        backgroundColor: theme.buttonDefaultBG,
    }),

    Change this LOC:

    this.props.large ? styles.buttonLarge : undefined,

    To:

    this.props.large ? styles.buttonLarge(Boolean(this.props.icon)) : undefined,
  6. narefyev91 commented on Oct 19, 2023

    @narefyev91
    Contributor

    @yh-0218 go ahead and create a PR - let's resolve this quick

  7. ginsuma commented on Oct 19, 2023

    @ginsuma
    Contributor

    @narefyev91, @yh-0218's solution won't fix #29759. That is the issue #29787 tried to fix.
    I'm sorry. I didn't test his solution carefully.

  8. amyevans commented on Oct 19, 2023

    @amyevans
    Contributor

    The blocker is stemming from #29759 so I'll assign the original authors/reviewers

  9. yh-0218 commented on Oct 19, 2023

    @yh-0218
    Contributor

    Hi, @narefyev91 PR is ready for review. Please check.

  10. cristipaval commented on Oct 19, 2023

    @cristipaval
    Contributor

    Thanks for triaging this @amyevans!

  11. cristipaval commented on Oct 19, 2023

    @cristipaval
    Contributor

    Removed the DeployBlockerCash label as this is a minor visual bug.

  12. cristipaval commented on Oct 27, 2023

    @cristipaval
    Contributor

    Given that the fix for this is production and it was a regression and no payment is due, I'm going to close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions