-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix missing video action bar on Safari #41218
Fix missing video action bar on Safari #41218
Conversation
Looks good from a design standpoint—just want to confirm that the expand button is supposed to always show even though the player controls only show on hover? That's how it's currently working for me, just want to double-check that's intended behavior. |
Yeah I would assume the expand/fullscreen arrow to show together with and like the controls not always be present. But that's how it works today in prod so I guess we should consider that a separate bug |
@dannymcclain @dubielzyk-expensify on production, the expand button is also always shown too. We didn't specify this during video player predesign and I implemented it this way 😅 If you think it should hide on hover, I can create a separate PR that changes it. Also, I was thinking about adding fade-in/out animations |
@Skalakid I get the loading icon infinitely shown for video player on mWeb Safari. Because of this, I cannot test the player on Safari mobile web. Any idea why this is so? Here is a test video on this. 40652-mweb-safari.mp4 |
@rojiphil Hmm that's interesting, I changed only the video styles. I tested it and the video loads normally on my side: Screen.Recording.2024-04-30.at.12.32.18.movWhat type of video have you sent, what's its' format? |
One is of
I agree this PR has not introduced this as I cross-checked with the latest main but curious to know if you would also face the same issue with these videos. |
Reviewer Checklist
Screenshots/VideosAndroid: Native40652-android-native-01.mp4Android: mWeb Chrome40652-mweb-chrome-01.mp4iOS: Native40652-ios-native-01.mp4iOS: mWeb Safari40652-mweb-safari-01.mp4MacOS: Chrome / Safari40652-web-safari-01.mp4MacOS: Desktop40652-desktop-01.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Video action bar is displayed on all platforms. LGTM
We did not find an internal engineer to review this PR, trying to assign a random engineer to #40652 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
I think we need someone from the @Expensify/design team to take a look since we are adding new styles. I wonder why no one got assigned. |
Ah perfect, then I am merging! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.70-5 🚀
|
Details
This PR fixes the bug with missing video action bar and
Expand
button in in-chat video previewFixed Issues
$ #40652
PROPOSAL:
Tests
Expand
button are visibleOffline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
N/A
Android: mWeb Chrome
N/A
iOS: Native
N/A
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
safari.web.mov
MacOS: Desktop
desktop.mov