Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 of 6 tasks
m-natarajan opened this issue May 16, 2024 · 48 comments
Closed
3 of 6 tasks

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

m-natarajan opened this issue May 16, 2024 · 48 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented May 16, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

recording.mp4
AUYH7500.1.MP4

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
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

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
Copy link
Contributor

tienifr commented May 17, 2024

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?

  • In here, we always use CONST.VIDEO_PLAYER.CONTROLS_STATUS.SHOW.

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

  1. We need to create a new state in here
const [controlStatusState, setControlStatusState] = useState(controlsStatus);
  1. Then create a function that allows us to toggle the above state, named toggleControlStatusState

  2. In here, call:

toggleControlStatusState()
  1. 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

  2. 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

Screen.Recording.2024-05-17.at.09.14.24.mov

@tienifr
Copy link
Contributor

tienifr commented May 17, 2024

Updated proposal to add the result.

@dubielzyk-expensify
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added the Overdue label May 20, 2024
@tienifr
Copy link
Contributor

tienifr commented May 20, 2024

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

@dubielzyk-expensify
Copy link
Contributor

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

@tienifr
Copy link
Contributor

tienifr commented May 20, 2024

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.

Copy link

melvin-bot bot commented May 20, 2024

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

@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label May 21, 2024
@melvin-bot melvin-bot bot changed the title Hide video playback controls on auto-playing videos [$250] Hide video playback controls on auto-playing videos May 21, 2024
Copy link

melvin-bot bot commented May 21, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 21, 2024
Copy link

melvin-bot bot commented May 21, 2024

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

Copy link

melvin-bot bot commented May 24, 2024

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

@melvin-bot melvin-bot bot added the Overdue label May 24, 2024
@rojiphil
Copy link
Contributor

@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?

@melvin-bot melvin-bot bot removed the Overdue label May 27, 2024
@tienifr
Copy link
Contributor

tienifr commented May 27, 2024

@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.

Copy link

melvin-bot bot commented May 28, 2024

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

@bfitzexpensify bfitzexpensify removed their assignment May 30, 2024
@bfitzexpensify bfitzexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels May 30, 2024
Copy link

melvin-bot bot commented May 30, 2024

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.

@alexpensify
Copy link
Contributor

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

@alexpensify
Copy link
Contributor

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

@alexpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

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 alexpensify added Weekly KSv2 and removed Monthly KSv2 labels Jul 9, 2024
@alexpensify
Copy link
Contributor

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

@alexpensify
Copy link
Contributor

alexpensify commented Jul 16, 2024

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

@alexpensify
Copy link
Contributor

Weekly Update: The PR is still being reviewed.

@alexpensify
Copy link
Contributor

Weekly Update: The PR is moving through reviews

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Aug 2, 2024
@alexpensify alexpensify added Weekly KSv2 and removed Monthly KSv2 labels Aug 7, 2024
@alexpensify
Copy link
Contributor

Moving back to Weekly since the PR is in the review process

@alexpensify
Copy link
Contributor

Weekly Update: QA steps are under review

@alexpensify alexpensify removed their assignment Aug 21, 2024
@alexpensify
Copy link
Contributor

@bfitzexpensify - I realized you are back online, so I'm exiting the stage here. Thanks!

@rojiphil
Copy link
Contributor

@bfitzexpensify @MariaHCD Since it is pay time here, I wanted to put forward a request to increase the compensation considering the amount of effort we spent in fine-tuning the video player.
Would $1000 sound fair?

Reasoning:

  1. Over a while, we identified improvements in 10 different use cases as captured in the test case description here
  2. This required us to repeat review and tests for 6 times as mentioned here (1, 2, 3, 4, 5, 6)

@bfitzexpensify
Copy link
Contributor

@MariaHCD I'll defer this to you - do you think the work described in the comment above is 4x what we'd usually expect?

@MariaHCD
Copy link
Contributor

MariaHCD commented Sep 2, 2024

I think the work done was definitely thorough and entailed quite a few improvements for the video player but I think it warrants a 2x increase (not 4x), in my opinion.

@bfitzexpensify
Copy link
Contributor

bfitzexpensify commented Sep 6, 2024

Thanks, Maria.

OK, let's go with a $500 payout.

Payment summary:

$500 to be paid to @rojiphil for C+ reviewer - offer sent via Upwork
$500 to be paid to @tienifr for contributor work via manual request

@rojiphil
Copy link
Contributor

rojiphil commented Sep 6, 2024

$500 to be paid to @rojiphil for contributor work - offer sent via Upwork

@bfitzexpensify Accepted offer as C+ reviewer. Awaiting payment. Thanks.

@bfitzexpensify
Copy link
Contributor

Paid out. Closing this one out.

@garrettmknight
Copy link
Contributor

$250 approved for @tienifr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants