-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Display Quick Action Button #38669
Display Quick Action Button #38669
Conversation
personalDetails: { | ||
key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the hook for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad. This is fine. I thought that this is current user's details which has a hook.
@shawnborton @dannymcclain @dubielzyk-expensify anyoneee wanna' take the quick action button for a spin? |
Looks good to me! Can you confirm why we have different avatars for the workspace requests though? |
Right, I'm saying, why is that avatar where it's just the workspace icon different from how it is displayed in the "Send to" field as well as the LHN item for it? |
I'm not quite sure I'm following. It looks like the mockups in the design (I think), and, as far as I can tell, it looks like the workspace avatar in the LHN (minus the user's own avatar as a subscript) |
Yes, this is the exact point I am trying to make. Why are we choosing a different avatar for the quick action button? I would think that we should use the identical icon that we use for sending something to a workspace chat, which happens to include the subscript avatar. @trjExpensify do you agree? |
|
Cool, I think it makes sense to be consistent here - basically if we are choosing to show that kind of avatar when you send an expense, we should reuse the same exact one from the shortcut menu too. So that looks good to me given we use that icon style everywhere else. |
Neat. All yours @rlinoz! |
Are we good to merge @shawnborton? |
I think so, yup |
case CONST.QUICK_ACTIONS.ASSIGN_TASK: | ||
return 'quickAction.assignTask'; | ||
default: | ||
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid crashes (even if we've missed some action type), we should have added some default fallback key here instead of an empty string. This caused #38972
return avatars.length <= 1 || ReportUtils.isPolicyExpenseChat(quickActionReport) ? avatars : _.filter(avatars, (avatar) => avatar.id !== props.session.accountID); | ||
} | ||
return []; | ||
}, [props.personalDetails, props.session.accountID, quickActionReport]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were missing policy
in the dependencies here, which caused #39045 where the avatar was not kept in sync if the workspace's avatar changed.
@@ -80,6 +144,47 @@ function FloatingActionButtonAndPopover(props) { | |||
|
|||
const prevIsFocused = usePrevious(props.isFocused); | |||
|
|||
const quickActionReport = useMemo(() => (props.quickAction ? ReportUtils.getReport(props.quickAction.chatReportID) : 0), [props.quickAction]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line created a bug where the icon and group name is not updated correctly. More details here #40536
Details
Fixed Issues
$ #38050
PROPOSAL:
Tests
Let's do this!
Offline tests
Same, but offline
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop