[HOLD for payment 2023-08-24] [CHECKLIST TO BE COMPLETED] [$1000] Workspace +N overlay has a smaller border radius than the workspace icon #23426
Comments
kavimuru
added
Daily
|
Triggered auto assignment to @jliexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.The workspace +N overlay has a smaller border-radius than the workspace icon. What is the root cause of that problem?In the code, the workspace icon and the +N overlay have the same border radius, which is 6. They also have the same size, which is 24, but somehow the overlay border radius appears smaller. Here is why: App/src/components/MultipleAvatars.js Lines 170 to 183 in b7d17e0 The second border radius is in Avatar. Lines 93 to 104 in b7d17e0 On the other hand, the overlay only has 1 border radius style. This View also has a border width of 2. App/src/components/MultipleAvatars.js Lines 199 to 215 in b7d17e0 The bigger the value, the bigger the radius. But as we also know, border radius is also affected by the element size. For the workspace icon, the first View has a border width of 2, which means the total width of the element is 24 + 2 (border left) + 2 (border right) = 28. while the second View width is still 24. This makes the first View radius smaller than the 2nd View.  The image above shows the radius difference between the parent View and the child View. In the case of the overlay, the width is also 28 (24 + 2 + 2).  The image above also shows the radius difference between the overlay and the workspace icon. Because the radius is different, we can notice the overlay background color. What changes do you think we should make in order to solve the problem?Avatar code has a lot of responsibilities and I think the most ideal solution is to refactor the Avatar and MultipleAvatar code so we don't have the nested View with multiple border radius, but I also think that requires a lot of effort.
View diff```diff diff --git a/src/components/MultipleAvatars.js b/src/components/MultipleAvatars.js index 3b20d99fa2..6e4ce933ca 100644 --- a/src/components/MultipleAvatars.js +++ b/src/components/MultipleAvatars.js @@ -206,9 +206,7 @@ function MultipleAvatars(props) { isInReportAction: props.isInReportAction, shouldUseCardBackground: props.shouldUseCardBackground, }), - - // Set overlay background color with RGBA value so that the text will not inherit opacity - StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity), + styles.bgTransparent, <- we need to set it to transparent to override the bg from getHorizontalStackedAvatarBorderStyle StyleUtils.getHorizontalStackedOverlayAvatarStyle(oneAvatarSize, oneAvatarBorderWidth), props.icons[3].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, props.icons[3].type) : {}, ]} @@ -219,6 +217,9 @@ function MultipleAvatars(props) { styles.alignItemsCenter, StyleUtils.getHeight(oneAvatarSize.height), StyleUtils.getWidthStyle(oneAvatarSize.width), + // Set overlay background color with RGBA value so that the text will not inherit opacity + StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity), + StyleUtils.getAvatarBorderStyle(props.size, props.icons[3].type), ]} > ### What alternative solutions did you explore? (Optional) **Alternative 1** To have the same radius between the icon parent and the icon itself, we should adjust the parent border radius by taking into account the border width. For example, the workspace icon border width is 2, so we add the current border radius, which is 6, with the border width (6 + 2 = 8). The parent (container) border radius will be 8. 1. Create a new StyleUtils called `getAvatarContainerBorderRadius(size, type, borderWidth)`. It will simply call `getAvatarBorderRadius` and add `borderWidth` to the result. 2. Replace the usage of `getAvatarBorderRadius` (on the icon and overlay) with `getAvatarContainerBorderRadius`
**Alternative 2**
I think this alternative solution is cleaner but requires more changes, but there is a catch.
The idea is to completely remove the border from the overlay. We want just a simple View that sits on top of the workspace icon with the same size and border radius.
1. Remove the overlay child View and move the height and width style to the parent.
https://github.com/Expensify/App/blob/b7d17e0fd33b83e9fbd45146b6123b6323ece79e/src/components/MultipleAvatars.js#L216-L223
2. Remove the border style of the overlay parent View. This will remove the border color and "unused" bg color.
https://github.com/Expensify/App/blob/b7d17e0fd33b83e9fbd45146b6123b6323ece79e/src/components/MultipleAvatars.js#L203-L208
3. Remove the second border style of the overlay parent View. (remove the borderWidth and borderStyle)
https://github.com/Expensify/App/blob/b7d17e0fd33b83e9fbd45146b6123b6323ece79e/src/styles/StyleUtils.js#L920-L928
4. Because we are removing the border from the overlay, we need to adjust the `left` positioning.
Before: `left: -(oneAvatarSize.width * 2 + oneAvatarBorderWidth * 2)`
After: `left: -(oneAvatarSize.width * 2 + oneAvatarBorderWidth)`
`oneAvatarBorderWidth` is the 4th workspace icon's right border width.
5. We need to somehow disable the tooltip of the icon behind the overlay because now the user can hover over the icon border.
 , OR if we want, we can also do a change so that user can't hover over the icon border, which I prefer. |
|
Yep, I can repro:
|
jliexpensify
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~01fb51d18c394675da |
melvin-bot
bot
added
the
Help Wanted
|
Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia ( |
|
Reposting the proposal because the prev one is not properly formatted. ProposalPlease re-state the problem that we are trying to solve in this issue.The workspace +N overlay has a smaller border-radius than the workspace icon. What is the root cause of that problem?In the code, the workspace icon and the +N overlay have the same border radius, which is 6. They also have the same size, which is 24, but somehow the overlay border radius appears smaller. Here is why: The workspace icon is a nested view that has 2 border-radius styles with the same value (6). The first border-radius is in MultipleAvatars. This View has a border width of 2. App/src/components/MultipleAvatars.js Lines 170 to 183 in b7d17e0 The second border radius is in Avatar. Lines 93 to 104 in b7d17e0 On the other hand, the overlay only has 1 border radius style. This View also has a border width of 2. App/src/components/MultipleAvatars.js Lines 199 to 215 in b7d17e0 The bigger the value, the bigger the radius. But as we also know, border radius is also affected by the element size. For the workspace icon, the first View has a border width of 2, which means the total width of the element is while the second View width is still 24. This makes the first View radius smaller than the 2nd View. The image above shows the radius difference between the parent View and the child View. In the case of the overlay, the width is also 28 (24 + 2 + 2). The image above also shows the radius difference between the overlay and the workspace icon. Because the radius is different, we can notice the overlay background color. What changes do you think we should make in order to solve the problem?So, I would like to propose this simple idea. This will make the Avatar have a width of 28 (24 + 2 + 2) and because we apply the border radius to the Avatar, the radius will be the same as the overlay. I put this as a main solution because the radius is consistent with other workspace icons. 
View diffdiff --git a/src/components/MultipleAvatars.js b/src/components/MultipleAvatars.js
index 3b20d99fa2..bf77bf0dc6 100644
--- a/src/components/MultipleAvatars.js
+++ b/src/components/MultipleAvatars.js
@@ -169,19 +169,20 @@ function MultipleAvatars(props) {
>
<View
style={[
- styles.justifyContentCenter,
- styles.alignItemsCenter,
- StyleUtils.getHorizontalStackedAvatarBorderStyle({
- isHovered: props.isHovered,
- isPressed: props.isPressed,
- isInReportAction: props.isInReportAction,
- shouldUseCardBackground: props.shouldUseCardBackground,
- }),
- StyleUtils.getHorizontalStackedAvatarStyle(index, overlapSize, oneAvatarBorderWidth, oneAvatarSize.width),
- icon.type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, icon.type) : {},
+ StyleUtils.getHorizontalStackedAvatarStyle(index, overlapSize),
+ StyleUtils.getAvatarBorderRadius(props.size, icon.type),
]}
>
<Avatar
+ iconAdditionalStyles={[
+ StyleUtils.getHorizontalStackedAvatarBorderStyle({
+ isHovered: props.isHovered,
+ isPressed: props.isPressed,
+ isInReportAction: props.isInReportAction,
+ shouldUseCardBackground: props.shouldUseCardBackground,
+ }),
+ StyleUtils.getAvatarBorderWidth(props.size),
+ ]}
source={icon.source || props.fallbackIcon}
fill={themeColors.iconSuccessFill}
size={props.size}
diff --git a/src/styles/StyleUtils.js b/src/styles/StyleUtils.js
index c8449d16d1..552be36012 100644
--- a/src/styles/StyleUtils.js
+++ b/src/styles/StyleUtils.js
@@ -878,20 +878,17 @@ function getKeyboardShortcutsModalWidth(isSmallScreenWidth) {
* @returns {Object}
*/
function getHorizontalStackedAvatarBorderStyle({isHovered, isPressed, isInReportAction = false, shouldUseCardBackground = false}) {
- let backgroundColor = shouldUseCardBackground ? themeColors.cardBG : themeColors.appBG;
+ let borderColor = shouldUseCardBackground ? themeColors.cardBG : themeColors.appBG; // removed the bg color because previously it is being used to cover the "hole" that is caused by the border radius difference between the parent and the avatar
if (isHovered) {
- backgroundColor = isInReportAction ? themeColors.highlightBG : themeColors.border;
+ borderColor = isInReportAction ? themeColors.highlightBG : themeColors.border;
}
if (isPressed) {
- backgroundColor = isInReportAction ? themeColors.highlightBG : themeColors.buttonPressedBG;
+ borderColor = isInReportAction ? themeColors.highlightBG : themeColors.buttonPressedBG;
}
- return {
- backgroundColor,
- borderColor: backgroundColor,
- };
+ return {borderColor};
}
/**
@@ -902,11 +899,9 @@ function getHorizontalStackedAvatarBorderStyle({isHovered, isPressed, isInReport
* @param {Number} borderRadius
* @returns {Object}
*/
-function getHorizontalStackedAvatarStyle(index, overlapSize, borderWidth, borderRadius) {
+function getHorizontalStackedAvatarStyle(index, overlapSize) {
return {
left: -(overlapSize * index),
- borderRadius,
- borderWidth,
zIndex: index + 2,
};
}
What alternative solutions did you explore? (Optional)Alternative 1
View diffdiff --git a/src/components/MultipleAvatars.js b/src/components/MultipleAvatars.js
index 3b20d99fa2..6e4ce933ca 100644
--- a/src/components/MultipleAvatars.js
+++ b/src/components/MultipleAvatars.js
@@ -206,9 +206,7 @@ function MultipleAvatars(props) {
isInReportAction: props.isInReportAction,
shouldUseCardBackground: props.shouldUseCardBackground,
}),
-
- // Set overlay background color with RGBA value so that the text will not inherit opacity
- StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity),
+ styles.bgTransparent, // we need to set it to transparent to override the bg from getHorizontalStackedAvatarBorderStyle
StyleUtils.getHorizontalStackedOverlayAvatarStyle(oneAvatarSize, oneAvatarBorderWidth),
props.icons[3].type === CONST.ICON_TYPE_WORKSPACE ? StyleUtils.getAvatarBorderRadius(props.size, props.icons[3].type) : {},
]}
@@ -219,6 +217,9 @@ function MultipleAvatars(props) {
styles.alignItemsCenter,
StyleUtils.getHeight(oneAvatarSize.height),
StyleUtils.getWidthStyle(oneAvatarSize.width),
+ // Set overlay background color with RGBA value so that the text will not inherit opacity
+ StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.overlay, variables.overlayOpacity),
+ StyleUtils.getAvatarBorderStyle(props.size, props.icons[3].type),
]}
>
<TextAlternative 2
|
|
Updated my proposal solution |
|
@bernhardoj's proposal looks good to me! 🎀 👀 🎀 C+ reviewed |
|
Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @thesahindia Please request via NewDot manual requests for the Reviewer role ($1000) |
|
📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app! |
melvin-bot
bot
added
Reviewing
|
PR is ready cc: @thesahindia |
|
🎯 ⚡️ Woah @thesahindia / @bernhardoj, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.49-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-08-10. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
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:
|
|
@jliexpensify , as @bernhardoj explained here this bug was not generated by the code changes he did. The problem is that the fix for this issue was supposed to be a 2 part fix, and we fixed just one. So I think we should not apply the regression penalty in this situation. |
|
@danieldoglas no worries, I can adjust the Payment for @bernhardoj . What about for @thesahindia ? EDIT - https://expensify.slack.com/archives/C01SKUP7QR0/p1691717578369759 We'll also pay out the C+ the full amount. |
|
Also, @thesahindia do we need to complete an updated checklist or anything? |
|
Paid in Upworks, job removed. @thesahindia - is a checklist needed? Also, please request $1000 in New.Dot. |
jliexpensify
changed the title
|
@thesahindia bump on checklist + ND payment request |
|
Bump @thesahindia - do we need a checklist? |
melvin-bot
bot
changed the title
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.54-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-08-24. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
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:
|
|
Sorry for the delay. I was out. Regression test steps - Prerequisite:
I couldn't find the PR that caused this issue. There were multiple changes so it's hard to track. P.S. I have requested the payment. |
|
Reviewed the details for @thesahindia. $1,000 approved for payment based on the BZ summary above. |
|
Closing this, I think everytihng is sorted! |
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:
Requirement: has >= 5 workspaces
To spot it easier, please zoom it
Expected Result:
The overlay border radius is the same as the workspace icon border radius
Actual Result:
The overlay border radius is smaller than the workspace icon border radius
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.44-0
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
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @bernhardoj
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690036975314749
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: