[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[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[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[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/31518

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

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[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 🙁

[cubuspl42]

@kacper-mikolajczak Would you catch up with relevant threads and work on a follow-up PR?

[kacper-mikolajczak]

Hi @cubuspl42, definitely! I am going to work on the solution as soon as I am done with current stuff 👍

[kacper-mikolajczak]

Here is a quick summary of the existing issue:

Hoverable indirectly through OfflineWithFeedback component needs to render an array of children and fails to do so resulting in a crash.

A few things that I would like to point out:

• TS rules (in form of Hoverable props type) that were introduced in order not to let the multiple children to be passed, are not respected in regular js files. That's why we still need to carefully parse the children.
• I am not sure if the logic of mapChildren helper that is being used to extract the child is what we actually want. It looks like it just takes the first item in the array and discards rest.

function mapChildren(children: ((isHovered: boolean) => ReactElement) | ReactElement | ReactElement[], callbackParam: boolean): ReactElement & RefAttributes<HTMLElement> {
    if (Array.isArray(children)) {
        return children[0];
    }
   ...
}

If we want to actually support the multiple children inside Hoverable, then we would need to change that as well.

Question is, how to handle multiple refs coming from the mapping the children?

• 🚫 hooking into first ref is not suitable because we would implicitly lost information (as done in current solution with mapping children)
• ❓ gathering ref from each child and hooking logic into each of them? That would fail whenever single children is needed from the parent (BoundsObserver case). Additionally, the behaviour of such solution is quite unclear - shall we track hover mutually for all the children passed or for each individually?
• 💡 keep the solution suggested in this PR and refactor all the components so we always have single child passed into Hoverable. That way user knows what element Hoverable hooks into. On the flip side, we would not need to make any workarounds but it would require deep understanding of current behaviour and testing against regression.

Let me know what you guys think 👍

CC @cubuspl42

[bernhardoj]

It looks like it just takes the first item in the array and discards rest.

I can help answer this one. We currently only allow a Hoverable to have a single child. Before the functional migration, we check if the children is an array and the length is 1 before taking the first index.

If it's > 1, then React.Children.only will give an error.

children as function could return an array of children, so the above and current mapChildren is actually have a flaw

[cubuspl42]

• 💡 keep the solution suggested in this PR and refactor all the components so we always have single child passed into Hoverable. That way user knows what element Hoverable hooks into. On the flip side, we would not need to make any workarounds but it would require deep understanding of current behaviour and testing against regression.

Sounds clean, but I'm aware of the risk. How many uses are we talking about?

[kacper-mikolajczak]

Thanks a lot for the insight @bernhardoj! 👍

In this case, if we want to keep the single child approach but also have possibility to provide single child wrapped in the array (e.g. OfflineWithFeedback example), then we would need to add a mapper for that.

I've added this functionality here: 76a200fa020e162fa6cb9da8acd0cfe6e2dd8fe4, but it was not enough to solve the issue.

While debugging, I came to the point where I tracked down what is causing that and it seems that the problem lies in this change: 87d8979687fd19a16eb685a342231ca60a006728. After I reverted the changes to BaseTooltip (it did clone the React element to pass the mouse event handler), it looked okay.

Question is, if we want to go back to previous approach, where we can pass onMouse* events through Hoverable props (thus avoiding cloning there) or shall we find different approach.

I am open to suggestions ^^

[cubuspl42]

I'm not sure. I liked the idea with ensuring a single child is passed to Hoverable.

[kacper-mikolajczak]

I've had a second more on this and it turned out that what was crashing the app is React.cloneElement inside BaseTooltip to acquire array as a children which resulted in an error.

I've posted the fix that we can use here .

It basically parse children in order to extract single child. Let me know what you guys think @cubuspl42 @bernhardoj

[cubuspl42]

I left a comment in that PR and stand by what I said earlier. I think we should fix the usages instead of hacking around.

[mountiny]

Kacper is out of office but pr review continues

[melvin-bot[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[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/33257
• https://github.com/Expensify/App/pull/34161

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

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[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[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[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