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

Video subjects #389

Merged
merged 17 commits into from
Jul 5, 2022
Merged

Video subjects #389

merged 17 commits into from
Jul 5, 2022

Conversation

chelseatroy
Copy link
Contributor

@chelseatroy chelseatroy commented Dec 22, 2021

What this change should allow you to do:

ON a swipe layout project, you can now include video subjects! In staging, you can test this on the video subject workflow of the "All Mobile Workflows" project.

Review Checklist

  • Does it work in Android and iOS?
  • Can you rm -rf node_modules/ && npm install and app works as expected?
  • Are tests passing?

My goals for this PR:

@chelseatroy chelseatroy marked this pull request as ready for review January 5, 2022 04:20
@chelseatroy chelseatroy requested a review from mcbouslog January 5, 2022 04:21
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Code looks good, works well on simulated iOS and Android devices.

One comment (related to the two instances the video component is used) related to repeating the video.

Not related to these changes, but maybe these changes cause it to continue to happen - in the ClassifierContainer.js, onFeedbackViewLayout, adding useNativeDriver: false after line 50 prevents a warning that otherwise continually logs.

src/components/FullScreenMedia.js Show resolved Hide resolved
src/components/classifier/SwipeableSubject.js Show resolved Hide resolved
@mcbouslog
Copy link
Contributor

I think this was noted elsewhere (Slack?), but some button text alignment may have changed:
Screen Shot 2022-01-14 at 3 36 18 PM
I can open a dedicated issue if that'd be helpful?

@mcbouslog
Copy link
Contributor

mcbouslog commented Jan 14, 2022

Possibly another separate issue to be opened, I'm getting the following error after swiping a video subject on Android:
Screen Shot 2022-01-14 at 5 08 56 PM
I think it may have to do with my subject queue, but not sure. I can open an issue and attempt to investigate more, but want to mention here if already aware of?

@chelseatroy
Copy link
Contributor Author

Hi @mcbouslog ! Yes, please open separate issue for the alignment, although anecdotally, we have had a few little alignment issues like that for as long as I've maintained this app (I clean them up as I see them). Not sure about that error an had a lot of trouble reproducing, so an issue there would be super helpful as well.

@mcbouslog
Copy link
Contributor

mcbouslog commented Apr 4, 2022

Possibly another separate issue to be opened, I'm getting the following error after swiping a video subject on Android: Screen Shot 2022-01-14 at 5 08 56 PM I think it may have to do with my subject queue, but not sure. I can open an issue and attempt to investigate more, but want to mention here if already aware of?

I'm unable to recreate this issue on the beta version of the app on my Android device, please disregard.

chelseatroy and others added 16 commits July 1, 2022 17:56
+ Still shows only a black screen in full screen mode
+ Still need to check that it works for single answer and multi answer question workflows
+ What do we do about the rectangle drawing task?
…various structural changes necessary to get the app running with those changes

+ Includes a sed script that runs after dependency installation to fix a typo in react native that gets fixed in version 0.65. When we upgrade to 0.65 we'll probably want to remove that second sed command in the package.json postinstall script string.
@chelseatroy chelseatroy merged commit 1304640 into master Jul 5, 2022
@lcjohnso lcjohnso deleted the video-subjects branch July 15, 2022 05:02
@mcbouslog
Copy link
Contributor

Closes #396

@mcbouslog
Copy link
Contributor

Closes #290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants