search

language-transfer / lt-app

React Native application for Language Transfer
https://www.languagetransfer.org/
Other
291 stars 26 forks source link

Adapt for iOS #27

Closed schonfeld closed 3 years ago

schonfeld commented 4 years ago

This PR adds support for iOS. Along this journey, I decided to invest some effort into making the codebase more readable, reusable, and efficient. In this process, I've converted virtually all files to Typescript, and added fairly opinionated linting to the commit process. npm run lint now not only validates eslint, but also builds the app using tsc in order to catch any typed errors.

I've also included some domain-specific types in interfaces.d.ts for our courses, lessons, & metadata.

Some packages seemed unnecessary to keep, such as rn-side-menu, which was easily dropped in favor of react-navigation's drawer navigator.

I've tested both platforms, and the app currently runs on both iOS & Android devices.

schonfeld commented 4 years ago

I've moved ./js/index.js from ./index.js. I couldn't get RN packager to find index.js in the js subdirectory. Is there some flag/config I'm not aware of? I think it might be easier to just leave index.js in root, for the sake for feature devs. @SyntaxBlitz, thoughts?

schonfeld commented 4 years ago

Well, I ended up getting a bit carried away from the scope. I've ported most of the types, interfaces, components & code over to typescript, and linted everything. I dropped some unnecessary modules like rn-sidemenu, and cleaned up everything else. Still needs a bit more layout work so that things look great on both {android, ios}. Should be done soon.

SyntaxBlitz commented 4 years ago

@schonfeld oh wow, thanks for all your work on this! I really appreciate the extra effort you're putting in to take it out of hackathon mode. Sorry for all the magic numbers and random inconsistencies :)

I'll have a look at this soon. I'm juggling a few things in my personal life but I'm very excited to see this come to fruition.

I saw you talked to Mihalis on the Facebook post. I don't think we have an Apple developer account set up yet but it'd be great if you can help him get one made. My general plan has been to make sure Mihalis has as much control as possible (in case I drop off the face of the planet). I'm not sure how signing works for iOS packages; I figure that'd be the biggest hurdle, just making sure he has the app signing keys for the future.

schonfeld commented 4 years ago

@SyntaxBlitz no rush there. I'm happy to help. I've been a LT fan for a quite some time. I still have some work left here, and I'd like to wrap it up by doing another Android pass, to make sure everything still looks good (and uniform) on both platforms. I'll update this PR's title once it's ready for some attention!

re Apple dev account -- I sent Mihalis a few messages on private messenger about what he'll need to get it going. If it's at all helpful, I can release a TestFlight version under my own dev account. We can circle back to this in a couple days.

schonfeld commented 4 years ago

@SyntaxBlitz I think it's ready for some device testing. I'll go ahead and install it on a few physical iOS devices. If you can do the same for Androids, that'd be great.

I fixed-up all of my commits so that this PR doesn't completely vomit all over the master branch....

Some notes for the near-future:

  1. We should setup Fastlane to build & deploy the app to both app stores.

  2. We should create a Github Action flow that runs npm run lint. Until we start writing some test code, simply linting the codebase should catch a fair amount of issues.

schonfeld commented 4 years ago

I got this live on TestFlight. If anyone has an iOS device & would like to beta test, please drop me a note.

schonfeld commented 4 years ago

@SyntaxBlitz this should be ready to merge. Lots of changes, upgrades, and updates. We've gone ahead and submitted to the iOS App Store. It might make sense to get on an audio call together with Mihalis & touch base?

I tested the app on a software Android emulator locally, but if you have a droid device, feel free to make sure everything still looks&works the same.

SyntaxBlitz commented 4 years ago

Thanks for the update! Planning to review the code today + tomorrow and test on my device. Then we should be good to merge :) and yeah, I've got pretty open availability for a call

schonfeld commented 4 years ago

I see what you mean about the pause/un-pause issue in Android. I'll get it fixed...

schonfeld commented 4 years ago

Alright, those scrubber issues should be fixed, including the play/pause bug. I had to upgrade our rn-track-player to use the latest dev branch. Their releases are pretty bad... v2 is about a year old, while v1.2.3 is the most recent. That said, even their most recent release doesn't contain anywhere near the correct TypeScript definitions...

schonfeld commented 3 years ago
schonfeld commented 3 years ago

Comments addressed. The only remaining item is to discuss whether or not we want to bundle first lessons in Android Bundle as we do in iOS. I'm in favor of consistency. But if you guys feel strongly about keeping AAB filesize tiny, we'd need to add a few more LOCs...

SyntaxBlitz commented 3 years ago

Comments addressed. The only remaining item is to discuss whether or not we want to bundle first lessons in Android Bundle as we do in iOS. I'm in favor of consistency. But if you guys feel strongly about keeping AAB filesize tiny, we'd need to add a few more LOCs...

how big are the files altogether? I'm still on the side of avoiding packaging them if we don't have to (even though it introduces inconsistency), but I could be convinced.

bufemc commented 3 years ago

It seems that this has been here for quite a while on hold. Can we somehow proceed? Maybe even just merge the commit where the "download all lessons" button is added, at least? ,-)

SyntaxBlitz commented 3 years ago

We'll also need to address #31 before merging. @schonfeld I can try to have a look this weekend if we're still stalled on this by then.

SyntaxBlitz commented 3 years ago

Merged these changes with #34, closing.