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

[HOLD for payment 2024-08-19] [Guided Setup Stage 3] Add the tooltip for Workspace Chat #45046

Closed
deetergp opened this issue Jul 9, 2024 · 46 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 NewFeature Something to build that is a new item.

Comments

@deetergp
Copy link
Contributor

deetergp commented Jul 9, 2024

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.

Screenshot 2024-07-09 at 9 56 59 AM

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:

$onyxData[] = [
    'onyxMethod' => Onyx::METHOD_MERGE,
    'key' => OnyxKeys::NVP_WORKSPACE_TOOLTIP, // workspaceTooltip
    'value' => [
        'shouldShow' => true,
    ],
],

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 OwnerCurrent Issue Owner: @stephanieelliott
@deetergp deetergp self-assigned this Jul 9, 2024
@trjExpensify trjExpensify changed the title Add the tooltip for Workspace Chat [Guided Setup Stage 3] Add the tooltip for Workspace Chat Jul 9, 2024
@trjExpensify trjExpensify moved this to Release 2: Summer 2024 (Aug) in [#whatsnext] #wave-collect Jul 9, 2024
@trjExpensify trjExpensify added the NewFeature Something to build that is a new item. label Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

Triggered auto assignment to @stephanieelliott (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Weekly KSv2 label Jul 10, 2024
@trjExpensify
Copy link
Contributor

Trying to confirm if this will be held on work in #44277. CC: @tienifr

@tienifr
Copy link
Contributor

tienifr commented Jul 11, 2024

#44277 and this share the same anchorAlignment mechanism but I faced several issues in #44277 that do not happen in this one. I think we can definitely bring the anchorAlignment implementation from #44277 here and get the PR right away to speed up the process, no need to hold. I can take this too.

@trjExpensify
Copy link
Contributor

But @tienifr does that mean the anchorAlignment approach isn't the right one if it doesn't work for #44277?

@tienifr
Copy link
Contributor

tienifr commented Jul 11, 2024

It worked perfectly, there's just another problem that didn't work in that PR.

@trjExpensify
Copy link
Contributor

Okay, can you post your proposal here for posterity? Also, @dukenv0307 can you be the C+ on this? Thanks!

@tienifr
Copy link
Contributor

tienifr commented Jul 12, 2024

Proposal

Please 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 EducationalTooltip from #40066 with the same approach there.

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 anchorAlignment property which mimics Popover's behavior:

anchorAlignment = {
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
},

image

By default anchorAlignment is Bottom center.

  1. Add anchorAlignment to Tooltip component tree
  2. Modify this function to accept anchorAlignment:

const createTooltipStyleUtils: StyleUtilGenerator<GetTooltipStylesStyleUtil> = ({theme, styles}) => ({

  1. Apply the EducationalTooltip for ReportActionCompose with shouldRender as the condition of first time viewing the chat (I'm not sure if the BE already returned this prop or we need to create a new Onyx prop) with the given content.

Specifically for step 2, the current logic to compute tooltip position in createTooltipStyleUtils already supports the Bottom center alignment. We need to handle other cases:

Vertical alignment

Modify shouldShowBelow to include the TOP vertical alignment (tooltip shown below target):

shouldShowBelow =
shouldForceRenderingBelow ||
yOffset - tooltipHeight - POINTER_HEIGHT < GUTTER_WIDTH + titleBarHeight ||
!!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight));

|| anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP

Horizontal alignment

Reuse the current logic to calculate initial tooltip left position which includes the distance from the left edge of screen to the target xOffset and avoids displaying too near to the edge of the screen horizontalShift and manualShiftHorizontal:

rootWrapperLeft = xOffset + horizontalShift + manualShiftHorizontal
LEFT

We just need to shift the pointer a bit to the right so it won't overflow the tooltip border:

Screenshot 2024-07-12 at 11 06 55
pointerWrapperLeft = POINTER_WIDTH / 2
RIGHT
  1. Move the pointer to the right by the tooltip width and shift left a bit so it won't overflow the tooltip border like this:
Screenshot 2024-07-12 at 11 14 52
pointerWrapperLeft = horizontalShiftPointer + (tooltipWidth - POINTER_WIDTH * 1.5)
  1. Move the tooltip to the right by the target width and shift left by itself so it won't get out of the target space like this:
Screenshot 2024-07-12 at 11 12 05
rootWrapperLeft += tooltipTargetWidth - tooltipWidth
CENTER

Just reuse the current logic:

pointerWrapperLeft = horizontalShiftPointer + (tooltipWidth / 2 - POINTER_WIDTH / 2)
rootWrapperLeft += tooltipTargetWidth / 2 - tooltipWidth / 2

Code changes are here.

POC

Screenshot 2024-07-12 at 11 46 12 Screenshot 2024-07-12 at 11 58 06

@dukenv0307
Copy link
Contributor

@trjExpensify I would love to work on this issue as C+. Thanks

@deetergp
Copy link
Contributor Author

Proposal looks good to me, how about you @dukenv0307?

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 13, 2024
@dukenv0307
Copy link
Contributor

LGTM as well @deetergp

@deetergp
Copy link
Contributor Author

Nice! Let's 🚀 @tienifr

@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

When viewing the chat for the first time

@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?

@deetergp
Copy link
Contributor Author

Good question, @tienifr. How do we do it for the other tool tips?

@trjExpensify
Copy link
Contributor

I believe for the quick action button we have a isFirstQuickAction boolean:

isFirstQuickAction?: boolean;

@trjExpensify
Copy link
Contributor

@deetergp did you give @tienifr what he needs here to move this on?

@deetergp
Copy link
Contributor Author

@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!

@tienifr
Copy link
Contributor

tienifr commented Jul 17, 2024

@deetergp Could you please grant me access to the doc tienifr032022@gmail.com?

@trjExpensify
Copy link
Contributor

@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.

@deetergp
Copy link
Contributor Author

Oh nice! I must have missed the memo on that one 😅

@deetergp deetergp added the Reviewing Has a PR in review label Jul 30, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

@deetergp, @stephanieelliott, @tienifr, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@deetergp
Copy link
Contributor Author

deetergp commented Aug 7, 2024

Settle down Melvin, we just merged the PR.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Aug 12, 2024
@melvin-bot melvin-bot bot changed the title [Guided Setup Stage 3] Add the tooltip for Workspace Chat [HOLD for payment 2024-08-19] [Guided Setup Stage 3] Add the tooltip for Workspace Chat Aug 12, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Aug 12, 2024

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:

  • @tienifr requires payment through NewDot Manual Requests
  • @dukenv0307 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Aug 12, 2024

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:

  • [@dukenv0307] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@stephanieelliott
Copy link
Contributor

Hey @dukenv0307 can you please propose a regression test when you get a chance?

@dukenv0307
Copy link
Contributor

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR: N/A
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug. Yes
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test:

  1. Log in as User A, an account that owns a workspace
  2. Invite B, a user that does not yet have an Expensify account
  3. Check B's email for the invitation
  4. Open the new.expensify.com link from the email in a different browser
  5. On A, open the workspace chat
  6. Verify a tooltip shows over the composer's Create button saying Get started! Submit your first expense or ¡Empecemos! Presenta tu primer gasto
  7. Press Create button
  8. Verify the tooltip disappears (or it will automatically disappear in 5 seconds)

Do we 👍 or 👎

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Aug 18, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

Payment Summary

Upwork Job

BugZero Checklist (@stephanieelliott)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@stephanieelliott
Copy link
Contributor

stephanieelliott commented Aug 20, 2024

Summarizing payment on this issue:

Upwork job is here: https://www.upwork.com/jobs/~0162602935c704dbe5

@melvin-bot melvin-bot bot removed the Overdue label Aug 20, 2024
@tienifr
Copy link
Contributor

tienifr commented Aug 20, 2024

@stephanieelliott The PR took a lot of times and efforts to implement (39 files changed). Should it be 500$? cc @dukenv0307 @deetergp

@melvin-bot melvin-bot bot added the Overdue label Aug 22, 2024
@stephanieelliott
Copy link
Contributor

Chatted with @deetergp in DM, we agree that it's fair to raise this issue ot $500 given the work involved. Updated the payment summary here, also updated your Upwork offer @tienifr -- please accept when you get a chance!

@melvin-bot melvin-bot bot removed the Overdue label Aug 23, 2024
@github-project-automation github-project-automation bot moved this from Release 2: Summer 2024 (Aug) to Done in [#whatsnext] #wave-collect Aug 23, 2024
@dukenv0307
Copy link
Contributor

Thanks @stephanieelliott, I have accepted the offer on Upwork

@tienifr
Copy link
Contributor

tienifr commented Aug 25, 2024

Updated the payment summary #45046 (comment), also updated your Upwork offer @tienifr

@stephanieelliott I actually receive payment via NewDot, I've requested there 👍

@garrettmknight
Copy link
Contributor

@tienifr paid $500 via NewDot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 NewFeature Something to build that is a new item.
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants