search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.51k stars 2.87k forks source link

[HOLD for payment 2022-12-22] [$1000] mWeb - Workspace - The error text message extends beyond the screen #13505

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

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. Login with the same account on 2 different devices
  2. On device 1 (mWeb/Chrome) go to settings > workspaces
  3. Open a workspace
  4. Now at device 2 (Web) go to the same workspace and delete it
  5. Immediately delete the same workspace on device 1 (mWeb/Chrome)

Expected Result:

The text error message does NOT extend beyond the screen

Actual Result:

The error text message extends beyond the screen

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.38.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: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/206871323-342b1f4f-705f-41a7-a4dc-5448e2531713.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ada03f592418685a
  • Upwork Job ID: 1602380086319108096
melvin-bot[bot] commented 1 year ago

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

situchan commented 1 year ago

Proposal

This is a regression from #13475 which fixes #13466

Here's the solution to fix this GH without reverting #13475:

https://github.com/Expensify/App/blob/main/src/components/OfflineWithFeedback.js#L105-L117

                <View style={StyleUtils.combineStyles(styles.offlineFeedback.error, props.errorRowStyles)}>
-                   <DotIndicatorMessage messages={props.errors} type="error" />
+                   <DotIndicatorMessage style={[styles.flex1]} messages={props.errors} type="error" />
                    <Tooltip text={props.translate('common.close')}>
                        <Pressable
                            onPress={props.onClose}
                            style={[styles.touchableButtonImage, styles.mr0]}
                            accessibilityRole="button"
                            accessibilityLabel={props.translate('common.close')}
                        >
                            <Icon src={Expensicons.Close} />
                        </Pressable>
                    </Tooltip>
                </View>
melvin-bot[bot] commented 1 year ago

⚠️ 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.

pecanoro commented 1 year ago

Taking over as CME to move things faster.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01ada03f592418685a

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

pecanoro commented 1 year ago

@situchan your proposal sounds good, did you make sure your proposal does not get any regressions related to the rest of the one-dot indicators? What about the original issue? Does the message still show?

situchan commented 1 year ago

@situchan your proposal sounds good, did you make sure your proposal does not get any regressions related to the rest of the one-dot indicators? What about the original issue? Does the message still show?

yes I tested all the cases. The issue happens only when parent view of DotIndicatorMessage has style with flexDirection: row and this case flex: 1 should be applied to DotIndicatorMessage

pecanoro commented 1 year ago

@situchan Great! Since it's a deploy blocker here, I will assign you so we can have a PR asap.

melvin-bot[bot] commented 1 year ago

📣 @situchan You have been assigned to this job by @pecanoro! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

pecanoro commented 1 year ago

@situchan Since it's a deploy blocker, do you think you can get a PR today in the next couple of hours so that we can merge? Otherwise, I will have to take over since it's blocking us.

pecanoro commented 1 year ago

Also, take into account that there is a bonus if a PR gets merged within 3 business days if that is an incentive to get it ready asap 😄

chiragsalian commented 1 year ago

@situchan so how's the progress here and when can we expect a PR?

situchan commented 1 year ago

@situchan so how's the progress here and when can we expect a PR?

in 30 min

situchan commented 1 year ago

@pecanoro PR is ready

KamoEllen commented 1 year ago

"Proposal"

Style prop allows the specification of a maxWidth value, which will constrain the text to the specified width and will cause it to wrap to additional lines if necessary.. If the message contains more than two lines of text or is wider than a specific number of pixels, it will be truncated and wrapped to additional lines as necessary. This ensures that the error message will not extend beyond the screen width and will remain readable.//Another way to solve this problem is to use the ScrollView component to create a scrollable container for the text.I have 6 possible solutions to this problem. The best approach will depend on your specific needs and the layout of your app.//Another method-A React Native API that calculates the width of the screen and set the width prop of your Text component accordingly. This will ensure that the text is always sized to fit the available space. I hope this helps!

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2022-12-20. :confetti_ball:

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

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

melvin-bot[bot] commented 1 year ago

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:

Santhosh-Sellavel commented 1 year ago

Regression caused by this https://github.com/Expensify/App/pull/13475, we have it found here already

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2022-12-22. :confetti_ball:

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

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

Santhosh-Sellavel commented 1 year ago

@maddylewis bump on payment!

pecanoro commented 1 year ago

No need for "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." since there is actually a bullet point for it already that was missed 😅

maddylewis commented 1 year ago

@Santhosh-Sellavel @situchan - can you apply to the job so that i can process payment? https://www.upwork.com/jobs/~01ada03f592418685a

situchan commented 1 year ago

@maddylewis applied. thank you

pecanoro commented 1 year ago

[@maddylewis] A regression test has been added or updated so that the same bug will not reach production again.

@maddylewis For the regression test you need to follow the steps here to double-check in TestRail.

Santhosh-Sellavel commented 1 year ago

@maddylewis Applied for the job!

mallenexpensify commented 1 year ago

Looks like the PR was merged the same day @situchan was hired (not sure I've seen that before, nice work) Paid @situchan $1500 which includes the 50% bonus. Hired @Santhosh-Sellavel can you please accept the job and reply here once you have?

Santhosh-Sellavel commented 1 year ago

Done!

mallenexpensify commented 1 year ago

Paid @Santhosh-Sellavel $1500. Closing. I closed the below issue in favor of this one too, they were fixed by same PR.

https://github.com/Expensify/App/issues/13510#issuecomment-1364421451

maddylewis commented 1 year ago

i appreciate the assist @mallenexpensify - thank you!