[HOLD for payment 2024-01-18] [HIGH] Clean up Hoverable component

[mountiny]

Coming from here

Problem

Hoverable definition contains mixed logic of a few states (disabled , scrolling, hasHoverSupport), which makes it harder to understand and maintain.

Solution

Here is a list of issues with potential improvements that could help with that:

1. Logic responsible for internal updates is interleaved with logic responsible for disabled and hoveredEnabled states of the component. This may lead to hard-to-spot bugs.
   • Split the component into two distinct paths, InactiveHoverable and ActiveHoverable (see linked PR). This way we don't care about handling the disabled logic inside actual Hoverable
2. Component children definition is not strict enough causing complicated logic for child retrieval
   • Component should only accept single ReactElement child or callback that returns such single child - if more than one element is needed to have a hovered mechanism, then wrap them into a mutual parent
3. Component subscribes to unnecessary events, such as onBlur - if Hoverable should be responsible for focus effects as well, it should be taken into account explicitly
   • Let’s discuss if we could safely remove the subscription to onBlur
4. There are two pairs of callbacks onHover* and onMouse* which distinction and role are not clearly visible (they are not the same but I am not sure if we need the distinction, e.g. onMouse* event can be taken directly from children passed to Hoverable)
   • Discuss if we could simplify it, so only one pair is needed (see linked PR)
5. There are a lot of Hoverable components on the page (lists’ items etc.) and each of them subscribes to scrolling event
   • Is there an alternative way for what we currently have?

Issue Owner

Current Issue Owner: @MitchExpensify

[melvin-bot]

Triggered auto assignment to @lschurr (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[mountiny]

cc @kacper-mikolajczak can you please comment here

[kacper-mikolajczak]

Hi @mountiny , I would like to work on this issue

[mountiny]

PR is in a review

[cubuspl42]

• Logic responsible for internal updates is interleaved with logic responsible for disabled and hoveredEnabled states of the component. This may lead to hard-to-spot bugs.

  • Split the component into two distinct paths, InactiveHoverable and ActiveHoverable (see linked PR). This way we don't care about handling the disabled logic inside actual Hoverable

✅

• Component children definition is not strict enough causing complicated logic for child retrieval

@kacper-mikolajczak Would you say a word or two about how this requirement was met?

• Component should only accept single ReactElement child or callback that returns such single child - if more than one element is needed to have a hovered mechanism, then wrap them into a mutual parent

✅

• Component subscribes to unnecessary events, such as onBlur - if Hoverable should be responsible for focus effects as well, it should be taken into account explicitly
  • Let’s discuss if we could safely remove the subscription to onBlur

@kacper-mikolajczak Analogically. Could we safely remove the subscription to onBlur?

• There are two pairs of callbacks onHover* and onMouse* which distinction and role are not clearly visible (they are not the same but I am not sure if we need the distinction, e.g. onMouse* event can be taken directly from children passed to Hoverable)

  • Discuss if we could simplify it, so only one pair is needed (see linked PR)

Done ✅

• There are a lot of Hoverable components on the page (lists’ items etc.) and each of them subscribes to scrolling event

  • Is there an alternative way for what we currently have?

@kacper-mikolajczak Analogically. Is there an alternative way for what we currently have?

[kacper-mikolajczak]

Component children definition is not strict enough causing complicated logic for child retrieval
Would you say a word or two about how this requirement was met?

The requirement has been already met after refactoring the component to TS. In this PR we simply removed unnecessary mapChildren function which indicated that the component can receive multiple children. Also, the TS typings for child was corrected.

Analogically. Could we safely remove the subscription to onBlur?
After the discussion, we decided not to remove the onBlur handler as it might revive old issues.

Analogically. Is there an alternative way for what we currently have?
Unfortunately, no findings on this one.

@cubuspl42 Let me know if that answered your concerns 👍

[cubuspl42]

Great, thanks!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

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

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.7-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Hoverable Component refactor #31518

If no regressions arise, payment will be issued on 2023-12-12. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @cubuspl42 requires payment
• @kacper-mikolajczak does not require payment (Contractor)

[melvin-bot]

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:

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

[cubuspl42]

The PR was reverted, we'll need to regroup 🙁

[melvin-bot]

@cubuspl42, @mountiny, @kacper-mikolajczak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mountiny]

@kacper-mikolajczak has been out the PR is moving along though, whats your ETA?

[kacper-mikolajczak]

Hi @mountiny, I am going to complete the PR today and hand it off to the reviewer 👍

[mountiny]

@roryabraham Assigning you here too since you have helped a lot with the issue and reviews

[melvin-bot]

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

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.24-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Refactor Hoverable component v2 #33257
• [NoQA] Quickfix: Hoverable refactor #33257 #34161

If no regressions arise, payment will be issued on 2024-01-18. 🎊

For reference, here are some details about the assignees on this issue:

• @cubuspl42 requires payment (Needs manual offer from BZ)
• @kacper-mikolajczak does not require payment (Contractor)

[melvin-bot]

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:

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

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @MitchExpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @cubuspl42 paid $(AMOUNT) via Upwork (LINK)
• Contributor: @kacper-mikolajczak is from an agency-contributor and not due payment

BugZero Checklist (@MitchExpensify)

• 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

[mountiny]

@500 to @cubuspl42 I believe

[MitchExpensify]

Automation didn't seem to create an Upwork offer - Created one here for you @cubuspl42 : https://www.upwork.com/nx/wm/offer/100533857

[cubuspl42]

Accepted 👍

[MitchExpensify]

Paid and contract ended, thanks @cubuspl42