[HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$250] Context/popover menus have incorrect border radius

[m-natarajan]

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.4.76-4
Reproducible in staging?: y
Reproducible in production?: y
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: @dannymcclain
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716913234239859

Action Performed:

1. Go to NewDot
2. Open context menu, Edit comment Global create (any context menu)

Expected Result:

Context/popover menus should use one of our predefined border radius variables. Not an engineer, but I would think componentBorderRadiusLarge (

App/src/styles/variables.ts

Line 28 in 95d328f

componentBorderRadiusLarge: 16,

), which is 16px.

Actual Result:

The border radius of context menus is 12px.

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

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015010f0cbb4319b2e
• Upwork Job ID: 1798225869486452736
• Last Price Increase: 2024-06-05
• Automatic offers:
• rojiphil | Reviewer | 102612211
• s77rt | Contributor | 102712893

Issue Owner

Current Issue Owner: @rojiphil

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[Krishna2323]

Proposal

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

What is the root cause of that problem?

The default value is always 12px in the file below.

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 47 in f5d28de

const createModalStyleUtils: StyleUtilGenerator<GetModalStylesStyleUtil> = ({theme, styles}) => ({

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

Update to the required number in all places where we use 12px.

What alternative solutions did you explore? (Optional)

[Nodebrute]

Proposal

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

Context/popover menus have incorrect border radius

What is the root cause of that problem?

We are using 12 px in these places

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 78 in f5d28de

borderRadius: 12,

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 132 in f5d28de

borderRadius: isSmallScreenWidth ? 0 : 12,

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 154 in f5d28de

borderRadius: 12,

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 200 in f5d28de

borderRadius: 12,

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

We should use 16 in all the above places

What alternative solutions did you explore? (Optional)

We can also use componentBorderRadiusLarge in all the above places

[dragnoir]

Proposal

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

Context/popover menus have incorrect border radius

What is the root cause of that problem?

Wrong border radius on

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 200 in deb4c79

borderRadius: 12,

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

We need to update only this value to 16 componentBorderRadiusLarge

App/src/styles/utils/generators/ModalStyleUtils.ts

Line 200 in deb4c79

borderRadius: 12,

it's for case CONST.MODAL.MODAL_TYPE.POPOVER:

If we update the other value, it will effect other component and not Context/popover menus

[melvin-bot]

@JmillsExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@JmillsExpensify Still overdue 6 days?! Let's take care of this!

[JmillsExpensify]

Opening up to the community given that we have proposals.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015010f0cbb4319b2e

[melvin-bot]

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

[rojiphil]

Thanks for all your proposals.
@dragnoir proposal LGTM as the border-radius of pop-over menu alone has to be updated. The other proposals (i.e. here and here) suggested changing at multiple places which is not correct.
🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @jasperhuangg, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @rojiphil 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @dragnoir You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

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

• Change modal border radius to 16px #43627

If no regressions arise, payment will be issued on 2024-07-02. 🎊

For reference, here are some details about the assignees on this issue:

• @rojiphil requires payment automatic offer (Reviewer)
• @s77rt requires payment automatic offer (Contributor)

[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:

• [@rojiphil / @s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@rojiphil / @s77rt] 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:
• [@rojiphil / @s77rt] 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:
• [@rojiphil / @s77rt] Determine if we should create a regression test for this bug.
• [@rojiphil / @s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

I don't think the checklist applies here. This is more like a design change than an actual bug

[jasperhuangg]

@s77rt Agree, I think we can skip the checklist. @mallenexpensify If we pay them out I think we can go ahead and close this one out. I'll check everything off the checklist just for consistency.

[mallenexpensify]

@rojiphil can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~015010f0cbb4319b2e

@s77rt , I'm planning to pay you via NewDot, I canceled the Upwork job.

[rojiphil]

can you please accept the job and reply here once you have?

@mallenexpensify Accepted. Thanks.

[melvin-bot]

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

• Change modal border radius to 16px #43627

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

• @rojiphil requires payment automatic offer (Reviewer)
• @s77rt requires payment automatic offer (Contributor)

[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:

• [@rojiphil / @s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@rojiphil / @s77rt] 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:
• [@rojiphil / @s77rt] 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:
• [@rojiphil / @s77rt] Determine if we should create a regression test for this bug.
• [@rojiphil / @s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mallenexpensify]

Damn it! We got the double post/title update thing happening again.

Contributor: @s77rt due $250 via NewDot.
Contributor+: @rojiphil paid $250 via Upwork

Unsure if we need/want a testcase for this, since it's design related. If so.. it'd be included in the monthly test that QA runs for design and edge cases. @rojiphil, can you complete the BZ checklist above plz?

@

[melvin-bot]

@rojiphil, @s77rt, @mallenexpensify, @jasperhuangg Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mallenexpensify]

@rojiphil can you complete the BZ checklist above plz?

[rojiphil]

@mallenexpensify Sorry for the delay here. Here is the BZ checklist

• [@rojiphil] The PR that introduced the bug has been identified. Link to the PR: Offending PR
• [@rojiphil] 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: Added comment
• [@rojiphil] 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: Not Required. Existing checklist includes the provision to involve design team during styling changes which should catch such issues
• [@rojiphil] Determine if we should create a regression test for this bug. : Since the changes are related to styling which is widely used, this is prone to regression. Let’s add regression test case for this.
• [@rojiphil] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Click on Global FAB button to show the popover menu
2. Verify that the border radius is of size 16 (This can be verified via Inspect window in console on web versions) (#Test 1)
3. Navigate to a report with comments
4. Invoke the context menu for a comment.
5. Verify that the border radius is of size 16 (#Test 2)
6. Navigate to Settings and edit Avatar image to show the context menu
7. Verify that the border radius is of size 16 (#Test 3)
8. Navigate to Workspace. For a given workspace, invoke 3Dots Menu popover and select Delete workspace to display confirmation modal dialog.
9. Verify that the border radius is of size 16 (#Test 4)

[mallenexpensify]

Thanks @rojiphil !

Commented that the test case is design/edge case and should likely be run monthly
https://github.com/Expensify/Expensify/issues/412266

[JmillsExpensify]

$250 approved for @s77rt