Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Dev: LHN - Border color around profile pic is different in LHN #29079

Closed
6 tasks done
kbecciv opened this issue Oct 9, 2023 · 16 comments
Closed
6 tasks done

[$500] Dev: LHN - Border color around profile pic is different in LHN #29079

kbecciv opened this issue Oct 9, 2023 · 16 comments
Assignees
Labels
External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Oct 9, 2023

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. Create a group
  2. Click on the group and verify border color around profile pic in LHN

Expected Result:

Border color is different

Actual Result:

Border color should not be different

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: Dev 1.3.79.3
Reproducible in staging?: n
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
Notes/Photos/Videos: Any additional supporting documentation

image (9)
image (8)
image (7)

RPReplay_Final1696655902.MP4
Screen_Recording_20231007_104615_New.Expensify.Dev.mp4
Screen_Recording_20231007_104703_Chrome.mp4
Screen.Recording.2023-10-07.at.10.34.04.AM.mov

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696655745191809

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01092e70102b992daf
  • Upwork Job ID: 1711339977280749568
  • Last Price Increase: 2023-10-09
@kbecciv kbecciv added the External Added to denote the issue can be worked on by a contributor label Oct 9, 2023
@melvin-bot melvin-bot bot changed the title Dev: LHN - Border color around profile pic is different in LHN [$500] Dev: LHN - Border color around profile pic is different in LHN Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01092e70102b992daf

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 9, 2023
@saranshbalyan-1234
Copy link
Contributor

Proposal

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

border color around focused profile pic is different in LHN

What is the root cause of that problem?

The problem is in the secondaryAvatarStyle being passed to the MultipleAvatars component.

<MultipleAvatars
icons={optionItem.icons}
isFocusMode={props.viewMode === CONST.OPTION_MODE.COMPACT}
size={props.viewMode === CONST.OPTION_MODE.COMPACT ? CONST.AVATAR_SIZE.SMALL : CONST.AVATAR_SIZE.DEFAULT}
secondAvatarStyle={[
StyleUtils.getBackgroundAndBorderStyle(themeColors.sidebar),
props.isFocused ? StyleUtils.getBackgroundAndBorderStyle(focusedBackgroundColor) : undefined,
hovered && !props.isFocused ? StyleUtils.getBackgroundAndBorderStyle(hoveredBackgroundColor) : undefined,
]}
shouldShowTooltip={OptionsListUtils.shouldOptionShowTooltip(optionItem)}
/>

When focused we are passing focusedBackgroundColor which is different from the activeComponentBG color

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

instead of focusedBackgroundColor we should pass activeComponentBG
Result
Screenshot 2023-10-07 at 3 11 00 PM

What alternative solutions did you explore? (Optional)

We can also change the value of styles.sidebarLinkActiveLHN.backgroundColor to themeColors.activeComponentBG's values

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@PiyushChandra17
Copy link

PiyushChandra17 commented Oct 9, 2023

Proposal

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

LHN - Border color around profile pic is different in LHN

What is the root cause of that problem?

We are using themeColors.sidebar here,

StyleUtils.getBackgroundAndBorderStyle(themeColors.sidebar),

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

We need to replace themeColors.sidebar with themeColors.appBG,

StyleUtils.getBackgroundAndBorderStyle(themeColors.appBG),

What alternative solutions did you explore? (Optional)

NA

Result:

Screenshot 2023-10-09 at 5 11 06 PM

@ishpaul777
Copy link
Contributor

this can be fixed with #29058

@Santhosh-Sellavel
Copy link
Collaborator

@saranshbalyan-1234's proposals LGTM!
C+ Reviewed
🎀 👀 🎀

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Oct 12, 2023
@saranshbalyan-1234
Copy link
Contributor

Hi @grgia friendly bump for review

@grgia
Copy link
Contributor

grgia commented Oct 12, 2023

@Santhosh-Sellavel this seems like a recent regression, have we determined if a recent PR caused this and the author should fix it?

@melvin-bot melvin-bot bot removed the Overdue label Oct 12, 2023
@ishpaul777
Copy link
Contributor

ishpaul777 commented Oct 12, 2023

Not sure but looks like a regression from #28277, styling issues from #28277 are consolidated in #29057 and being taken care of in #29428 cc @hayata-suenaga

@saranshbalyan-1234
Copy link
Contributor

Nope,I checked and this won't take care of this scenario at all

@hayata-suenaga
Copy link
Contributor

I feel like I remember we talked about this as a regression. Checking in the consolidated issue

@hayata-suenaga
Copy link
Contributor

@Santhosh-Sellavel please check the linked consolidated issue and let the Bug Zero team know if we should close this issue

@kosmydel kosmydel mentioned this issue Oct 16, 2023
96 tasks
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

@grgia, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Santhosh-Sellavel
Copy link
Collaborator

I guess we can close this one, as it is being handled as part of #29057

@hayata-suenaga
Copy link
Contributor

sorry I thought I was assigned to this issue 😓 @grgia please open the issue if you feel like it needs some more work

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Oct 17, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants