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.

Sign up for GitHub

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

Fix GBR and RBR show up at the same time in the LHN #18137

Merged
merged 3 commits into from
May 2, 2023
Merged

Fix GBR and RBR show up at the same time in the LHN #18137

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2660cbe
fix GBR and RBR show up at the same time in the LHN
hungvu193 Apr 28, 2023
4d0b22b
sync with main
hungvu193 May 1, 2023
688ba8e
hide pinned button while LHN has green dot
hungvu193 May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/components/LHNOptionsList/OptionRowLHN.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ const OptionRowLHN = (props) => {
const focusedBackgroundColor = styles.sidebarLinkActive.backgroundColor;

const avatarTooltips = !optionItem.isChatRoom && !optionItem.isArchivedRoom ? _.pluck(optionItem.displayNamesWithTooltips, 'tooltip') : undefined;
const shouldShowGreenDotIndicator = optionItem.isUnreadWithMention || (optionItem.hasOutstandingIOU && !optionItem.isIOUReportOwner);
const hasBrickError = optionItem.brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
const shouldShowGreenDotIndicator = !hasBrickError && (optionItem.isUnreadWithMention || (optionItem.hasOutstandingIOU && !optionItem.isIOUReportOwner));

return (
<OfflineWithFeedback
Expand Down Expand Up @@ -201,13 +202,13 @@ const OptionRowLHN = (props) => {
</Text>
</View>
) : null}
{optionItem.brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR && (
{hasBrickError && (
<View style={[styles.alignItemsCenter, styles.justifyContentCenter]}>
<Icon
src={Expensicons.DotIndicator}
fill={colors.red}
height={variables.iconSizeSmall}
width={variables.iconSizeSmall}
height={variables.iconSizeNormal}
width={variables.iconSizeNormal}
/>
</View>
)}
Expand All @@ -217,6 +218,14 @@ const OptionRowLHN = (props) => {
style={[styles.flexRow, styles.alignItemsCenter]}
accessible={false}
>
{shouldShowGreenDotIndicator && (
<Icon
height={variables.iconSizeNormal}
width={variables.iconSizeNormal}
src={Expensicons.DotIndicator}
fill={colors.green}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think themeColors.success is better here.
@hungvu193 is there any reason for replacing with colors.green?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's from previous commit, cc @puneetlath, since I think that's diff from you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @aimane-chnaif! We should not import colors directly 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we are doing that in multiple places though. I think we should put up a PR to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @aimane-chnaif! We should not import colors directly 😬

Agree, this is important for dark/light theme switch which will be implemented very soon

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should create a separate issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that we should be using success here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah weird. I thought I had changed this to success in a previous PR.

/>
)}
{optionItem.hasDraftComment && (
<View
style={styles.ml2}
Expand All @@ -225,8 +234,7 @@ const OptionRowLHN = (props) => {
<Icon src={Expensicons.Pencil} height={16} width={16} />
</View>
)}
{shouldShowGreenDotIndicator && <Icon src={Expensicons.DotIndicator} fill={themeColors.success} />}
{optionItem.isPinned && (
{!shouldShowGreenDotIndicator && optionItem.isPinned && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.chatPinned')}
Expand Down