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

replace react-native-video with expo-av #18812

Merged
merged 11 commits into from Aug 9, 2019

Conversation

@songgao
Copy link
Member

commented Aug 6, 2019

No description provided.

@songgao songgao requested a review from keybase/react-hackers Aug 6, 2019

@jzila
Copy link
Contributor

left a comment

A question and a suggestion.

@@ -3,7 +3,7 @@ import * as Kb from '../../../../../common-adapters/mobile.native'
import * as Styles from '../../../../../styles'
import logger from '../../../../../logger'
import {Props} from './image-render.types'
import Video from 'react-native-video'
import {Video as ExpoVideo} from 'expo-av'

This comment has been minimized.

Copy link
@jzila

jzila Aug 7, 2019

Contributor

Why rename it?

This comment has been minimized.

Copy link
@songgao

songgao Aug 7, 2019

Author Member

Oh, I guess this one doesn't need rename. Will fix.

}
playingVideo ? this.videoRef.current.pauseAsync() : this.videoRef.current.playAsync()
return {playingVideo: !playingVideo}
})

This comment has been minimized.

Copy link
@jzila

jzila Aug 7, 2019

Contributor

Can you switch this to hooks?

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 7, 2019

Member

Prefer to not refactor this whole component for this PR

@mmaxim

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Did you try unfurls of various types that have video?

@songgao

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

Did you try unfurls of various types that have video?

@mmaxim I tried video attachment and giphy unfurl. Anything else I should test?

@songgao songgao force-pushed the songgao/HOTPOT-315 branch from c593dec to bf28bb2 Aug 9, 2019

@mmaxim

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Should be good

@mmaxim

mmaxim approved these changes Aug 9, 2019

@songgao songgao merged commit 25cae7c into master Aug 9, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@songgao songgao deleted the songgao/HOTPOT-315 branch Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.