Conversation
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.43-20 🚀
|
| const styles = useThemeStyles(); | ||
| const StyleUtils = useStyleUtils(); | ||
| const [loadComplete, setLoadComplete] = useState(false); | ||
| const isVideo = Str.isVideo(source); |
There was a problem hiding this comment.
This line caused crash - #36834
source is not always string, i.e. svg
|
|
||
| // Converts milliseconds to 'minutes:seconds' format | ||
| const convertMillisecondsToTime = (milliseconds: number) => { | ||
| const date = new Date(milliseconds); |
There was a problem hiding this comment.
Using the local Date object caused the time format issues. #37216 (comment)
| const updateCurrentlyPlayingURL = useCallback( | ||
| (url) => { | ||
| if (currentlyPlayingURL && url !== currentlyPlayingURL) { | ||
| pauseVideo(); |
There was a problem hiding this comment.
Not directly caused this issue but related pauseVideo is being called even if the video context is already destroyed. This is causing error in console. ref: #37142 (comment)
| icon: Expensicons.Download, | ||
| text: translate('common.download'), | ||
| onSelected: () => { | ||
| downloadAttachment(); |
There was a problem hiding this comment.
Coming from #36833, we shouldn't show the Download button for the local file preview
|
This PR caused this bug - Play&Pause buttons don't function in preview mode if expanded the video before RCA explained here - #36873 (comment) |
| const downloadAttachment = useCallback(() => { | ||
| currentVideoPlayerRef.current.getStatusAsync().then((status) => { | ||
| const sourceURI = `/${Url.getPathFromURL(status.uri)}`; | ||
| fileDownload(sourceURI); | ||
| }); | ||
| }, [currentVideoPlayerRef]); |
There was a problem hiding this comment.
This PR caused this bug:
RCA explained here - #40647 (comment)
| const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); | ||
| const isCurrentlyURLSet = currentlyPlayingURL === url; | ||
|
|
||
| const togglePlayCurrentVideo = useCallback(() => { |
| if (_.isEqual(attachments, attachmentsFromReport)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This early return caused the state variable page to be stale (at 0) where it should have been updated to -1 if the attachments array is empty
| const updatePlaybackSpeed = useCallback( | ||
| (speed) => { | ||
| setCurrentPlaybackSpeed(speed); | ||
| currentVideoPlayerRef.current.setStatusAsync({rate: speed}); |
There was a problem hiding this comment.
This line caused this bug:
#40646
Here's the RCA:
#40646 (comment)
| text: speed.toString(), | ||
| onSelected: () => { | ||
| updatePlaybackSpeed(speed); | ||
| }, |
There was a problem hiding this comment.
We should have taken care of the highlighting of the sub item once selected. This caused #42425
|
|
||
| VIDEO_PLAYER: { | ||
| POPOVER_Y_OFFSET: -30, | ||
| PLAYBACK_SPEEDS: [0.25, 0.5, 1, 1.5, 2], |
There was a problem hiding this comment.
Turns out that PLAYBACK_SPEEDS required some additional granularity which led to this issue:
We fixed the issue by adding a wider range of PLAYBACK_SPEEDS, more details in the ^ issue / proposal.
| const pan = Gesture.Pan() | ||
| .onBegin((event) => { | ||
| runOnJS(setIsSliderPressed)(true); | ||
| runOnJS(checkVideoPlaying)(onCheckVideoPlaying); | ||
| runOnJS(pauseVideo)(); | ||
| runOnJS(progressBarInteraction)(event); | ||
| }) | ||
| .onChange((event) => { | ||
| runOnJS(progressBarInteraction)(event); | ||
| }) | ||
| .onFinalize(() => { | ||
| runOnJS(setIsSliderPressed)(false); | ||
| if (!wasVideoPlayingOnCheck.value) { | ||
| return; | ||
| } | ||
| runOnJS(playVideo)(); | ||
| }); |
There was a problem hiding this comment.
We should have used runOnJS(true) throughout the gesture. Not using that caused #51181
| useEffect(() => { | ||
| if (menuItems.length === 0) { | ||
| return; | ||
| } | ||
| setEnteredSubMenuIndexes([]); | ||
| setCurrentMenuItems(menuItems); | ||
| }, [menuItems]); |
There was a problem hiding this comment.
After updating the menuItems we should have updated the focusedIndex as well, otherwise we'd have two highlighted items: the new menuitems selected item and the stale focusedIndex (from the previous selected item).
Coming from #50224
|
Well, we forgot to handle offline case in fullscreen view, which caused #39424 |
20 participants
Details
Fixed Issues
$ #20471
$ #32770
PROPOSAL: #20471
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.native.webm
Android: mWeb Chrome
android.web.webm
iOS: Native
ios.native.mp4
iOS: mWeb Safari
ios.web.mp4
MacOS: Chrome / Safari
web_H.265.mp4
MacOS: Desktop
desktop.mov