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.14k stars 2.63k forks source link

[$250] Hide video playback controls on auto-playing videos #42323

Open m-natarajan opened 2 months ago

m-natarajan 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: 1.4.74-4 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @dubielzyk-expensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1715816175965469

Action Performed:

  1. On a new account, tap green (+) button
  2. Tap "Track Expense"
  3. Observe bottom sheet w/video about how to Track expense playing with controls obscuring the video

Expected Result:

The controls on the video shouldn't have the controls showing by default

Actual Result:

For videos auto-playing on mobile devices the controls should only show once the user taps the video. In the attachment modal it's worth showing the playback controls, but they should still fade out after 2s

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/570c0bb5-b93f-4cc9-9f05-045b6b218bbf

https://github.com/Expensify/App/assets/38435837/bcea0cd1-d667-49e1-a24a-205166f726d1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01db5e669ade628193
  • Upwork Job ID: 1792927198372831232
  • Last Price Increase: 2024-05-28
  • Automatic offers:
    • rojiphil | Contributor | 102529999
    • tienifr | Contributor | 102638224
Issue OwnerCurrent Issue Owner: @rojiphil
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @bfitzexpensify (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.

tienifr commented 2 months ago

Proposal

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

For videos auto-playing on mobile devices the controls should only show once the user taps the video. In the attachment modal it's worth showing the playback controls, but they should still fade out after 2s

What is the root cause of that problem?

  1. Then create a function that allows us to toggle the above state, named toggleControlStatusState

  2. In here, call:

    toggleControlStatusState()
  3. Then in here, use the above state to display the controls. We need to add a flag to just apply this change if needed to keep the backward compatibility, for example, a flag named shouldToggleControlOnPress

  4. If we want to display the controls then fade it out after 2s, in step 2, the toggleControlStatusState can be:

    const controlsOpacity = useSharedValue(0);
    const controlsAnimatedStyle = useAnimatedStyle(() => ({
        opacity: controlsOpacity.value,
    }));
    const toggleControlStatusState = () => {
        if (controlStatusState === CONST.VIDEO_PLAYER.CONTROLS_STATUS.SHOW) {
            setControlStatusState(CONST.VIDEO_PLAYER.CONTROLS_STATUS.HIDE);
        } else {
            setControlStatusState(CONST.VIDEO_PLAYER.CONTROLS_STATUS.SHOW);
            controlsOpacity.value = 1
            controlsOpacity.value = withTiming(0, {duration: 2000}, ()=> setControlStatusState(CONST.VIDEO_PLAYER.CONTROLS_STATUS.HIDE))
        }
    };

    then apply the controlsAnimatedStyle to the VideoPlayerControls components

    What alternative solutions did you explore? (Optional)

    NA

Result

https://github.com/Expensify/App/assets/113963320/cbd07cdd-865e-44d6-9203-2840dd0d377f

tienifr commented 2 months ago

Updated proposal to add the result.

dubielzyk-expensify commented 1 month ago

In the proposal here the controls fade out too quickly. When tapping the player, the controls should stay visible fully for 2 seconds (or 2.5 seconds) and fade out for 500 ms.

tienifr commented 1 month ago

@dubielzyk-expensify Yes, these durations can be changed when implementing the PR

dubielzyk-expensify commented 1 month ago

Cool. What does that mean? Is this not the PR to implement it?

tienifr commented 1 month ago

Yes, the video attached to my proposal demonstrates how my solution works. All the delay times you mentioned, such as staying fully visible for 2-2.5 seconds and fading out for 500 ms, can be optimized. If I am assigned to this task, I will create a PR to implement these adjustments.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

rojiphil commented 1 month ago

@tienifr Why do we need another state controlStatusState when we can use shouldPlay to determine if we need to show the controls initially or not?

tienifr commented 1 month ago

@rojiphil

when we can use shouldPlay to determine if we need to show the controls initially or not?

I don't think so. The controls are displayed based on controlStatus, not a shouldPlay, see here. Please correct me me I was wrong.

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @alexpensify (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.

bfitzexpensify commented 1 month ago

Adding a second BZ team member to keep an eye on this while I'm ooo until June 11th.

Currently in the proposal review stage.

melvin-bot[bot] commented 1 month ago

πŸ“£ @rojiphil πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 month ago

@rojiphil @alexpensify @bfitzexpensify 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!

rojiphil commented 1 month ago

I will review and share an update on this tomorrow my day

alexpensify commented 1 month ago

@rojiphil any news here? Thanks!

melvin-bot[bot] commented 1 month ago

@rojiphil, @alexpensify, @bfitzexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 month ago

@rojiphil, @alexpensify, @bfitzexpensify Huh... This is 4 days overdue. Who can take care of this?

rojiphil commented 1 month ago

yep. This is pending from my end. I will review and update on this tomorrow my day.

rojiphil commented 1 month ago

when we can use shouldPlay to determine if we need to show the controls initially or not?

I don't think so. The controls are displayed based on controlStatus, not a shouldPlay, see here. Please correct me me I was wrong.

Ok. I think we should not depend on shouldPlay to decide on displaying controls.

rojiphil commented 1 month ago

This is an improvement to an existing video player and @tienifr proposal is good enough to get this moving. Fine-tuning the implementation and design team approval can be done in the PR stage. πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

πŸ“£ @tienifr πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

tienifr commented 1 month ago

@rojiphil PR https://github.com/Expensify/App/pull/43349 is ready

alexpensify commented 1 month ago

Weekly update: The PR is ready for review.

alexpensify commented 4 weeks ago

Weekly update: The PR is going through the review process.

alexpensify commented 3 weeks ago

Weekly update: The PR is moving along through the process with no delays.

alexpensify commented 2 weeks ago

Weekly Update: More feedback in the PR, but moving forward

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @rojiphil, @alexpensify, @MariaHCD, @bfitzexpensify, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

alexpensify commented 1 week ago

Weekly Update: Moving back to weekly. @rojiphil the PR is ready for another review. Design confirmed the recent updates are good.

alexpensify commented 10 hours ago

Weekly Update: The PR is moving along through the review process.