[Regression test steps needed][$1000] BUG: Add attachment send button is too large

[neil-marcellini]

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

---

Action Performed:

1. Log in with any account
2. Open a chat and click the plus button to add an attachment, select a file to upload

Expected Result:

The send button is centered and has a normal width

Actual Result:

The send button stretches across the whole modal

Workaround:

Ignore the visual oddity

Platform:

All? Needs testing.

• Web
• iOS
• Android
• Desktop App
• Mobile Web

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

Expensify/Expensify Issue URL:
Issue reported by: @neil-marcellini
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669828932546429

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016b240866a2f9a693
• Upwork Job ID: 1598035611785265152

[melvin-bot]

Triggered auto assignment to @kevinksullivan (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[allroundexperts]

What is the required width @neil-marcellini?

[neil-marcellini]

cc @Expensify/design

[Pujan92]

Proposal

To make it normal at center postion we can update the style for the button.

App/src/styles/styles.js

Lines 472 to 474 in 2881f18

	buttonConfirm: {
	margin: 20,
	},

alignItems: 'center',

[shawnborton]

As for the button, we want it to be full width on mobile but for desktop, maybe we just give it a larger min-width (like 300?) and center it like it's shown above?

Looks like we need to update the background color behind the PDF as well.

[Puneet-here]

Proposal

Add attachmentButtonBigScreen to styles.js

attachmentButtonBigScreen:{
        minWidth: 300,
        alignSelf: 'center'
    }

Pass {[styles.buttonConfirm, this.props.isSmallScreenWidth ? {} : styles.attachmentButtonBigScreen]} at style

App/src/components/AttachmentModal.js

Line 281 in 1f71832

style={[styles.buttonConfirm]}

small screen

big screen

We need to passkey={this.props.isSmallScreenWidth}. If we want the button size to change when decreasing screen size. Because of it the button will re render when screen size changes and the needed styles will be applied

[akshayasalvi]

Proposal

In the styles.js update the buttonConfirm with the following styles:

  buttonConfirm: {
    alignSelf: `center`,
    minWidth: 300
  }

Web Version

iOS version

[shawnborton]

For mobile/small devices, we should have the button extend to full width. Only desktop/wide devices should get the min-width.

[Puneet-here]

@shawnborton, can you please check my proposal

[shawnborton]

Yup that works for me, but let's have our contributor team file and review first.

[Pujan92]

With the below solution, we can handle the small and large devices with a single class.

buttonConfirm: {
        padding: 20,
        width: '100%',
        maxWidth: 360,
        alignSelf: 'center',
    },

b8fa075e-11e9-4b2b-897d-6c645ff678d0.webm

[grgia]

Adding the external label now

[melvin-bot]

Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016b240866a2f9a693

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

[melvin-bot]

Current assignee @grgia is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @kevinksullivan is eligible for the Bug assigner, not assigning anyone new.

[thesahindia]

@Puneet-here's proposal looks good to me. It handles small screen devices too.

🎀👀🎀 C+ reviewed

cc: @grgia

[Pujan92]

@thesahindia do you think this is more suitable for this change where a same class is handling both devices?

[allroundexperts]

Proposal

@thesahindia @shawnborton I agree with @Puneet-here proposal. He however missed the incorrect background colour in the pdf preview screen. This can be fixed by:

diff --git a/src/components/AttachmentModal.js b/src/components/AttachmentModal.js
index 7895b119b..f1831bc09 100755
--- a/src/components/AttachmentModal.js
+++ b/src/components/AttachmentModal.js
@@ -275,10 +275,10 @@ class AttachmentModal extends PureComponent {
 
                     {/* If we have an onConfirm method show a confirmation button */}
                     {this.props.onConfirm && (
-                        <Animated.View style={StyleUtils.fade(this.state.confirmButtonFadeAnimation)}>
+                        <Animated.View style={[StyleUtils.fade(this.state.confirmButtonFadeAnimation), styles.buttonConfirmContainer]}>
                             <Button
                                 success
-                                style={[styles.buttonConfirm]}
+                                style={[styles.buttonConfirm, this.props.isSmallScreenWidth ? undefined : styles.attachmentButtonBigScreen]}
                                 textStyles={[styles.buttonConfirmText]}
                                 text={this.props.translate('common.send')}
                                 onPress={this.submitAndClose}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index adedd13af..87282326e 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -469,6 +469,11 @@ const styles = {
         marginRight: 22,
     },
 
+    attachmentButtonBigScreen: {
+        alignSelf: 'center',
+        minWidth: 300,
+    },
+
     buttonConfirm: {
         margin: 20,
     },
@@ -1822,7 +1827,7 @@ const styles = {
 
     imageModalPDF: {
         flex: 1,
-        backgroundColor: themeColors.modalBackground,
+        backgroundColor: themeColors.componentBG,
     },
 
     PDFView: {
@@ -1830,7 +1835,7 @@ const styles = {
         // It's being used on Web/Desktop only to vertically center short PDFs,
         // while preventing the overflow of the top of long PDF files.
         display: 'grid',
-        backgroundColor: themeColors.modalBackground,
+        backgroundColor: themeColors.componentBG,
         width: '100%',
         height: '100%',
         justifyContent: 'center',

Result

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.36-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Attachment button size change #13243

If no regressions arise, payment will be issued on 2022-12-15. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@thesahindia / @grgia] The PR that introduced the bug has been identified. Link to the PR:
• [@thesahindia / @grgia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@thesahindia / @grgia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@kevinksullivan] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/254500

[melvin-bot]

@kevinksullivan, @grgia, @thesahindia, @Puneet-here Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kevinksullivan]

Sorry for the delay here. @Puneet-here @thesahindia I just hired both of you for the job. Can you let me know once you've accepted to I can pay this out?

[kevinksullivan]

And can someone fill out the checklist items as well before we close this one out?

[thesahindia]

Accepted, thanks!

[thesahindia]

And can someone fill out the checklist items as well before we close this one out?

It wasn't a regression ( just an improvement) so we are good here.

[mallenexpensify]

Paid @thesahindia and @Puneet-here $1500, inc. the 50% bonus for expediency.
Thanks!
I think we still need to add regression steps to TestRail, assigned myself to hopefully help with those next week.

[melvin-bot]

@kevinksullivan, @mallenexpensify, @grgia, @thesahindia, @Puneet-here Huh... This is 4 days overdue. Who can take care of this?

[thesahindia]

No actions are needed from me, so unassigning.

[mallenexpensify]

Confirming regression test steps here

[mallenexpensify]

Didn't get feedback in Slack, thinking of adding the below, will do tomorrow after leaving here for a day
Recommendation: Update to add a regression step to
Compose box - Attachments (images) as new step #4 `Verify the width of the send button doesn't extend to the far edges of the screen on desktop, web and wide devices.

Compose box - Attachments (PDF) as new step #4 `Verify the width of the send button doesn't extend to the far edges of the screen on desktop, web and wide devices.

[melvin-bot]

@kevinksullivan, @mallenexpensify, @grgia, @Puneet-here Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mallenexpensify]

Regression test steps added https://github.com/Expensify/Expensify/issues/254500
@thesahindia @grgia , can you check off the other three boxes here then close this?
#13193 (comment)

[thesahindia]

It was an improvement and not a bug so I think we can skip them.

[mallenexpensify]

K, closing then. @grgia plz reopen if you disagree