Call Options menu doesn't dynamically scale/position when the window size is changed

[michaelhaxhiu]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Upwork job link - https://www.upwork.com/jobs/~01ba0dff1eec1e5790

---

Expected Result:

When you resize the Expensify Cash window, the Call Options menu should dynamically scale within the window.

Actual Result:

When you resize the Expensify Cash application, the Call Options menu does not properly scale with the window. Sometimes it gets cut off, the position is off, and the spacing is wrong.

Action Performed:

1. Open Expensify.cash on Web or Desktop
2. Log into your account
3. Click into a chat with another user
4. Click the 📞 phone icon in the top-right, and see the Call Option menu
5. Click on any edge of the window and change the size of the window. See that the Call Option menu isn't dynamically moving with the size of the window.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Yes, Expensify is still usable and you can size the window in a way that allows you to select the options.

Platform:

Where is this issue occurring?

• Web
• Desktop App
• iOS
• Android
• Mobile Web

Version Number: tested most recently on 1.0.22-1 (1.0.22-1)
Notes/Photos/Videos: See GIF recording below:

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/159934

View all open jobs on Upwork

[michaelhaxhiu]

Posted to Upwork today.

[MelvinBot]

Triggered auto assignment to @tgolen (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[aliabbasmalik8]

@michaelhaxhiu

The solution to this issue to update option box x,y coordinates when we resize the window. so to do this we need to add this code in the VideoChatButtonAndMenu component

import getPlatform from '../libs/getPlatform';

componentDidMount() {
   if (getPlatform() === 'web' || getPlatform() === 'desktop') {
        window.addEventListener('resize', this.measureVideoChatIconPosition);
    }
}

componentWillUnmount() {
   if (getPlatform() === 'web' || getPlatform() === 'desktop') {
        window.removeEventListener('resize', this.measureVideoChatIconPosition);
    }
}

and measureVideoChatIconPosition function already in code
https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/components/VideoChatButtonAndMenu.js#L59-L65

VIDEO

• ON WEB: https://recordit.co/O633uOvxDg
• ON DESKTOP: https://recordit.co/7LNDs93iRV

Thanks

[tgolen]

@aliabbasmalik8 Can you please update your proposal to conform to our cross-platform strategy (which can be found in the README of the repo).

Also, are there any performance concerns with your solution, and should it maybe use throttling or debouncing?

[aliabbasmalik8]

@tgolen Here is another solution
https://github.com/Expensify/Expensify.cash/blob/ba700bfa48c9b46cbd5e2bc9e05547207de6dcf1/src/components/VideoChatButtonAndMenu.js#L91-L94

We need to just update anchorPosition in these ways and it's working fine

anchorPosition={{
      right: 55,
      top: 52,
}}

according to this solution, we have no need for this function as well as no need videoChatIconPosition state value
https://github.com/Expensify/Expensify.cash/blob/ba700bfa48c9b46cbd5e2bc9e05547207de6dcf1/src/components/VideoChatButtonAndMenu.js#L59-L65

VIDEO:

• ON WEB: https://recordit.co/AXQNYAyMy8
• ON DESKTOP: https://recordit.co/B4I9woMKFk

NOTE: Can update this solution #2517 (comment) according to your requirements but I think the new solution is more accurate.
Looking for your suggestion

[tgolen]

@Luke9389 Would you mind chiming in on the above proposal to see what you think since you originally wrote some of this?

[parasharrajat]

Just a few thoughts, I think the solution to this issue should be applied to POPOVER. So that it works for all menus like this.

1. Emoji Picker
2. Call.
3. Right-click Context menu.
   ...more.

[aliabbasmalik8]

Just a few thoughts, I think the solution to this issue should be applied to POPOVER. So that it works for all menus like this.

1. Emoji Picker
2. Call.
3. Right-click Context menu.
   ...more.

Yes we can add default values in Popover component.
https://github.com/Expensify/Expensify.cash/blob/ba700bfa48c9b46cbd5e2bc9e05547207de6dcf1/src/components/Popover/index.js#L11

[Luke9389]

When I wrote this we were aware of this issue and decided to leave it. @roryabraham and I had discussed refactoring our popover menus and knew that this would be addressed then.

@aliabbasmalik8's most recent proposal resembles my initial approach to this problem. We sided against that because it absolutely positions the menu on the screen rather than "attaching" the menu to the button.

I think we should close this issue and address this on an Internal PR to refactor all of our Popover Menus. I know this is something @roryabraham had expressed interest in doing.

[tgolen]

@roryabraham what are your thoughts on this? How involved would it be to refactor the popovers? What would the refactor be doing?

[roryabraham]

Sorry, meant to reply to this yesterday.

The idea behind the refactoring @Luke9389 is referring to would be to create a generic menu component which would be reused between the CreateMenu, the VideoChatButtonAndMenu, etc... However, I sort of think that some of those menu components have diverged enough that the refactoring doesn't make sense anymore. Even so, the anchorPosition is always going to be supplied by the parent, so imo DRYing up the menus isn't the best way to solve this problem.

In regards to this issue, I think it can still be external. The task at hand is to remeasure the position of the Popovers' anchor point when the window dimensions change, and to then pass the updated anchorPosition to the Popovers.

I think that the best solution here is to refactor withWindowDimensions (or create a new wrapper component) to take in an onDimensionsChange prop that is debounced to execute every 200ms or so when the window dimensions change. Then, in each of the places we use onLayout or measureInWindow to dynamically determine a popover location, use that HOC or wrapper component to remeasure the position of the menu's anchor point and pass the updated anchorPosition prop to the Popover.

[roryabraham]

That means that I don't think the solution to this can be applied in the Popover itself as @parasharrajat has suggested, because it is the responsibility of the parent component to provide the anchor position, and the Popover itself shouldn't be changing its own props.

[roryabraham]

Looking into it some more, we could probably still do the generic menu component refactoring like @Luke9389 and I talked about – basically take BaseCreateMenu and make it a reusable component to DRY stuff up a bit. But I think that's a separate issue because it would still take in an anchorPosition as a prop, and it would still be the responsibility of the parent components to manage that piece of data. Hopefully that makes sense.

[parasharrajat]

Yeah.anchorPosition should be managed by the parent. But then we have to manage the same logic at multiple places so I suggested moving that into Popover. Is it possible that we pass the ref of anchor down to Popover to manage its positions with some offset props to adjust it from a parent?

WithWindowDiminensions should be refactored at the moment, it is keeping the js thread busy unnecessarily. It should only emit when there is a resize event, oppose to what it does currently. Also, Debouncing would be good.

[roryabraham]

Is it possible that we pass the ref of anchor down to Popover to manage its positions with some offset props to adjust it from a parent?

It's possible, yes, but that seems quite convoluted to me.

WithWindowDiminensions should be refactored at the moment, it is keeping the js thread busy unnecessarily. It should only emit when there is a resize event, oppose to what it does currently.

There is also the concern that we don't want to create a bunch of duplicate onDimensionsChange listeners. Maybe we should consider nixing withWindowDimensions entirely, and instead leverage Onyx as the single source-of-truth for window dimensions and onDimensionsChange callbacks. @tgolen curious what you think of that? We could still provide the window width, height, and isSmallScreenWidth to the components that are currently using withWindowDimensions via withOnyx, but then we would have only one global onDimensionsChange listener.

Also, Debouncing would be good.

Of course, I agree debouncing would be good. I suggested 200ms because my rule-of-thumb (taken from some random StackExchange thread) is that it takes 250ms for the human eye to register visual changes/movement, so 200ms is as infrequent a debounce as possible to still have changes appear instant.

[tgolen]

I really like the idea of storing this data in Onyx and let components subscribe to it whenever they need it. I like the debouncing idea as well. The way I think that could work is like this:

1. In Expensify.js use Dimensions.addEventListener()
2. When a dimension change happens, fire off a new action DimensionListener.set()
3. The new action handles the debounce (making sure to execute immediately for the first call) and then stores the value into Onyx
4. Any parents that are rendering a popover would subscribe to the new Onyx key and use those props to calculate the anchorPosition for any popovers
5. Bonus: replace withWindowDimensions everywhere

Yeah?

[parasharrajat]

I think it would be overkill. Any specific reason for sure keeping it in persistent storage? Instead, create a top-level provider for Dimensions and consuming it in withWindowDimensions could be an option.

[roryabraham]

Instead, create a top-level provider for Dimensions

@parasharrajat For now, Onyx is the only mechanism we have in place for global event emission, and it just happens to rely on persistent storage. I've toyed with the idea of creating a subset of Onyx keys which aren't saved, and that could allow us to use Onyx as a pure event emitter for data that doesn't need to be persisted. Doing so could potentially reduce the footprint of the app and speed up event emission for those keys. But I think that solution would be overkill. So I'd suggest we just use Onyx as-is for now, and if there is some problem where we need to reduce the bundle size or we're seeing performance issues with onDimensionsChange event handling, then we can revisit.

Another alternate solution would be to use some other mechanism for global event emission, such as Node.js' EventEmitter, but again I feel it might be overkill introduce a new code pattern in the codebase when we can just use Onyx.

[michaelhaxhiu]

Looking for an update as 1 week as elapsed, which means the job is now priced at $500.00 (instead of the original $250.00). 💰

@aliabbasmalik8 and @parasharrajat, do either of you intend to take this job using the latest Onyx method suggested by @roryabraham & @tgolen?

[parasharrajat]

@michaelhaxhiu I think someone is already working on this one so I didn't pick it.

I can look at it today.

[michaelhaxhiu]

@parasharrajat I'm certain this job is not yet assigned. 😺

• @aliabbasmalik8 has expressed interest first and can choose to take on this job. But I'd like to hear a confirmation from that individual based on the latest discussions and onyx method, as they haven't commented in about 8 days. So let's give them 1 day to respond here.

• If they don't respond in the next day, I'm happy to give you the option to take this job if you are open to it @parasharrajat!

[aliabbasmalik8]

@michaelhaxhiu
Thanks for waiting for me but I didn't understand the latest suggested solution so no more interested in this job.
Thanks

[michaelhaxhiu]

@aliabbasmalik8 no problem and thanks for being responsive. In case you are interested in working another job, please take a look at our Upwork job list.

@parasharrajat I'm going to assign you and hire you in Upwork, can you please send an offer on the post here?

[parasharrajat]

@michaelhaxhiu All set.

[michaelhaxhiu]

Hired ✅. Feel free to start on a PR in the next few days @parasharrajat

[marcaaron]

Hello, Sorry I missed this conversation, but I just want to chime in here because I'm a little lost on a few points:

1. It doesn't seem like this issue should require any refactor of withWindowDimensions and I'm afraid we are over-engineering this solution.
2. There is no reason to have "a single on dimensions change listener" as no one has asked if there is actual overhead to having multiple. The listeners are delegated (at least on web) and I think we might be trying to solve a problem that doesn't exist.
3. I'm not sold on the idea to move this stuff into Onyx for a few of reasons:
   1. We are adding an extra layer of complexity
   2. The change will need to go through AsyncStorage before being passed to subscribers which may slow it down
   3. We probably do not want this information persisted and there is no concept of data we should not persist in Onyx

Separately, there is another contributor working on debouncing withWindowDimensions so these proposed changes will clash with that (also not sure how they address the problem we are solving here in any case - it feels tacked on).

Looking at the PR I think all we really needed to do here was:

1. Add a Dimensions.addEventListener() in ReportActionCompose.js and VideoChatButtonAndMenu.js
2. Update the popover position

Can we please try that simple solution first and see if there are any negative effects?

[parasharrajat]

@marcaaron Agree. reverting the changes for withWindowDimensions.
> Separately, there is another contributor working on debouncing withWindowDimensions

I will skip this as well.

Waiting for others' input...

[marcaaron]

FWIW there was some previous conversation about refactoring withWindowDimensions here. But came to the conclusion that it's hard to justify the value.

[kidroca]

I agree with @marcaaron this is purely UI related and does not need to be persisted each time dimension change

I don't know the exact intention for Onyx but typically you would store app state that you cannot derive from somewhere else

Being a react-native app - JS time is precious - no matter how fast Onyx becomes it's seems unnecessary to move this handling there:

• Dimension -> Onyx -> component
• vs
• Dimension -> component

[kidroca]

@roryabraham

Is it possible that we pass the ref of anchor down to Popover to manage its positions with some offset props to adjust it from a parent?

It's possible, yes, but that seems quite convoluted to me.

I think this was the best idea so far. And it's exactly how Material-UI have interfaced it - providing a ref to a element makes sure the popup always points to that element: https://material-ui.com/components/popover/#anchor-playground and accounts for resizes as well as the item getting moved to a completely different location due to layout changes

I'm optimistic there would be a react-native library for this exact purpose so that it doesn't have to be coded in E.cash

[parasharrajat]

Everyone has different opinions but at last, the best would be the one that the team understands and like to maintain it.

[marcaaron]

I'm optimistic there would be a react-native library for this exact purpose so that it doesn't have to be coded in E.cash

There was a lot of discussion around why we did not use this library (or another), so we are unlikely to revisit it at this point.