-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2024-08-19] [Guided Setup Stage 3] Add the tooltip for Workspace Chat #45046
Comments
Triggered auto assignment to @stephanieelliott ( |
It worked perfectly, there's just another problem that didn't work in that PR. |
Okay, can you post your proposal here for posterity? Also, @dukenv0307 can you be the C+ on this? Thanks! |
ProposalPlease re-state the problem that we are trying to solve in this issue.Add tooltip for Workspace chat What is the root cause of that problem?This is a new feature What changes do you think we should make in order to solve the problem?We can reuse the Additionally, in order for the tooltip to align to the right of the target and the pointer to the left of the tooltip, we need to implement App/src/components/PopoverWithMeasuredContent.tsx Lines 43 to 46 in 456c8da
By default
Specifically for step 2, the current logic to compute tooltip position in Vertical alignmentModify App/src/styles/utils/generators/TooltipStyleUtils/index.ts Lines 171 to 174 in 58e2575
|| anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP Horizontal alignmentReuse the current logic to calculate initial tooltip left position which includes the distance from the left edge of screen to the target rootWrapperLeft = xOffset + horizontalShift + manualShiftHorizontal
|
@trjExpensify I would love to work on this issue as C+. Thanks |
Proposal looks good to me, how about you @dukenv0307? |
LGTM as well @deetergp |
Nice! Let's 🚀 @tienifr |
@deetergp How could we know the user is viewing the chat for the first time? Does the BE return this or do we need to introduce a new Onyx value? |
Good question, @tienifr. How do we do it for the other tool tips? |
I believe for the quick action button we have a App/src/types/onyx/QuickAction.ts Line 19 in fef5dfc
|
@trjExpensify @tienifr I have updated the Tooltip section of the doc with details on what to expect when deciding whether or not to show the tooltip. Please let me know if you've got any remaining questions. Thanks! |
@deetergp Could you please grant me access to the doc tienifr032022@gmail.com? |
@tienifr isn't C+, so hasn't got an NDA in place to access design docs. Let's get that into the GH issue OP. |
Oh nice! I must have missed the memo on that one 😅 |
@deetergp, @stephanieelliott, @tienifr, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Settle down Melvin, we just merged the PR. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.18-10 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 2024-08-19. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Hey @dukenv0307 can you please propose a regression test when you get a chance? |
BugZero Checklist:
Regression test:
Do we 👍 or 👎 |
Payment Summary
BugZero Checklist (@stephanieelliott)
|
Summarizing payment on this issue:
Upwork job is here: https://www.upwork.com/jobs/~0162602935c704dbe5 |
@stephanieelliott The PR took a lot of times and efforts to implement (39 files changed). Should it be 500$? cc @dukenv0307 @deetergp |
Thanks @stephanieelliott, I have accepted the offer on Upwork |
@stephanieelliott I actually receive payment via NewDot, I've requested there 👍 |
@tienifr paid $500 via NewDot |
Part of the Extend Onboarding to Invited Users project
Main issue: https://github.com/Expensify/Expensify/issues/392615
Doc section: https://docs.google.com/document/d/1EX1tKSkhffpqP4TXD7iKIPKWnU5U8ELv32j-QTtXH2g/edit#heading=h.bfsayh3fuyk8
Project: #wave-collect
Feature Description
When a user is invited to a Workspace as a non-admin, when they respond to the invitation email, they should be signed directly into the app and taken to their workspace chat. When viewing the chat for the first time, they should see a tool tip hovering above the Create button that says "Get started! Submit your first expense" that gets dismissed when they click on the button.
Frontend Changes (from doc)
In order to know when to show the tooltip, look for an onyx key called workspaceTooltip with a property of shouldShow set to true. This property will only be sent on their first sign-in. Once they click on the Create button or mouse into the composer, we should dismiss the tooltip and set workspaceTooltip.shouldShow to false. It will not be sent again on subsequent sign-ins. See the backend documentation for more details.
Backend Changes (from doc)
When a user has been invited to a workspace as a non-admin, we are going to want to show them a tooltip on their first signing encouraging them to submit their first expense. In order for the frontend to know this is their first sign-in we will check that their isInviteOnboardingComplete is false and inviteType is workspace and then also include an additional key in the onyx updates from this method:
When the user clicks the Create button or click into the composer field, we will dismiss the tooltip and set workspaceTooltip.shouldShow to false. The NVP will no longer be sent on subsequent sign-ins.
Manual Test Steps
Note: These won't be manually testable till the following backend GHs are done and deployed
https://github.com/Expensify/Expensify/issues/411150
https://github.com/Expensify/Expensify/issues/411149
https://github.com/Expensify/Expensify/issues/411148
https://github.com/Expensify/Expensify/issues/411146
Follow the steps for Invited to workspace as a non-admin from the design doc.
Automated Tests
Not sure if we can test for this.
Issue Owner
Current Issue Owner: @stephanieelliottThe text was updated successfully, but these errors were encountered: