Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.51k stars 2.86k forks source link

[$250] Android – Sign in–Sound option is missing when Get started modal video opened to full screen #47295

Closed IuliiaHerets closed 2 months ago

IuliiaHerets commented 2 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.19-4 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR https://github.com/Expensify/App/pull/46795 Email or phone of affected tester (no customers): ponikarchuks+00312824@gmail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in with a new Gmail account and click on Join button
  3. Complete the Onboarding flow
  4. In Get started video, note video is muted
  5. Enable video to full screen

Expected Result:

Sound option is present

Actual Result:

Sound option is missing

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/5f7a1ecc-738a-4c59-ac2b-9df584710e67

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a9e36c7f9220ebf4
  • Upwork Job ID: 1826259795924336728
  • Last Price Increase: 2024-08-28
Issue OwnerCurrent Issue Owner: @ikevin127
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 2 months ago

We think that this bug might be related to #vip-vsb

IuliiaHerets commented 2 months ago

@abekkala FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 2 months ago

@abekkala Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 months ago

@abekkala Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01a9e36c7f9220ebf4

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)

ikevin127 commented 2 months ago

👓 Looking for proposals (if reproducible).

huult commented 2 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Sound option is missing when Get started modal video opened to full screen

What is the root cause of that problem?

In full-screen mode, the video player is rendered at the system level, above any React Native components, so adding custom UI elements on top of the video can be challenging.

What changes do you think we should make in order to solve the problem?

To display custom UI components like a volume control in full-screen mode, we'll need to manage the full-screen mode manually rather than relying on presentFullscreenPlayer. This way, we maintain control over the video component and can overlay custom UI elements. Something like that:

+ <Modal visible={isFullscreen} supportedOrientations={['landscape', 'portrait']} onRequestClose={exitFullscreen}>
<Video
            ref={videoPlayerRef}
            style={[styles.w100, styles.h100, videoPlayerStyle]}
            videoStyle={[styles.w100, styles.h100, videoStyle]}
            source={{
                // if video is loading and is offline, we want to change uri to "" to
                // reset the video player after connection is back
                uri: !isLoading || (isLoading && !isOffline) ? sourceURL : '',
            }}
            shouldPlay={shouldPlay}
            useNativeControls={false}
            resizeMode={resizeMode as ResizeMode}
            isLooping={isLooping}
            onReadyForDisplay={(e) => {
                if (isCurrentlyURLSet && !isUploading) {
                    playVideo();
                }
                onVideoLoaded?.(e);
                if (shouldUseNewRate) {
                    return;
                }
                videoPlayerRef.current?.setStatusAsync?.({rate: currentPlaybackSpeed});
            }}
            onPlaybackStatusUpdate={handlePlaybackStatusUpdate}
            onFullscreenUpdate={handleFullscreenUpdate}
            volume={volume.value ? volume.value : 0}
        />
+ </Modal>
melvin-bot[bot] commented 2 months ago

@abekkala, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

ikevin127 commented 2 months ago

@huult Thank you for the proposal. I wasn't able to test your solution as the implementation is not specific enough for me to get a working prototype in order to verify whether it works. If I just wrap the Video component with the given Modal as the given DIFF specifies, the videos stop working and present an infinite loading spinner.

Can you please provide a working test branch in order for me to verify this ?

[!important] I opened up a discussion on Slack about this issue and whether or not it's worth addressing this issue given that most devices have a volume / mute button. Will bump the discussion as I posted it over the weekend and it did not get any attention.

Bumped on Slack here.

melvin-bot[bot] commented 2 months ago

@abekkala @ikevin127 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abekkala commented 2 months ago

vsb is on hold. This is now 2 weeks old. I'm going to label not priority and close for now