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

Launch Library 2 support #35

Merged
merged 4 commits into from Aug 5, 2020
Merged

Launch Library 2 support #35

merged 4 commits into from Aug 5, 2020

Conversation

derkweijers
Copy link
Contributor

@derkweijers derkweijers commented Aug 4, 2020

Since Launch Library 1 is deprecated and will stop working in the future, I took the liberty to update the app to use Launch Library 2, from The Space Devs. I'm a partner developer within this group (Spaceflight News API; thanks for using it!). I have your app installed for quite some time now, and I really like the clean design.

What I did was simply rename some of the properties that have a different name in LL2. Since LL2 doesn't have things like netstamps, I've also done quite a few Date-parsings to get the NET to seconds. For most (if not all) time values, I've used the NET time. Some other things that are good to know:

  • Descriptions are not always available. This is now checked when entering the "Details" screen. Check the upcoming Gaofen launch for an example;
  • There are no longer mission wiki's in the response. I've changed this to LSP wiki pages;
  • The "To be Determined" section might be obsolete now;
  • I have not tested notifications;
  • One of the tests is failing (the ResultCard one). I'm not sure why, so I've skipped it for now but you might wanna take a look at that :)

It's good to keep in mind that LL2 is free to use for 300 calls per day, per IP.

Please, let me know if you have any questions!

@Illu
Copy link
Owner

Illu commented Aug 4, 2020

Hey! Thanks a lot for opening this Pull Request and for your kind comments 😄 I did get the notification about the new API on the Discord server but didn't find the time to upgrade the code.

Looking at the analytics, 300 calls / day and IP should be enough, maybe we'll add a new warning in the app just in case someone reaches that limit.

I'll test your changes later today, thanks again for contributing!

This will be the perfect opportunity to send a new version to the App Store, as some bug fixes are still on master but not deployed yet.

@Illu Illu self-assigned this Aug 4, 2020
@Illu Illu added the enhancement New feature or request label Aug 4, 2020
@derkweijers
Copy link
Contributor Author

You are very welcome! 😄

During the pod installations, some dependencies were updated as well. I can also check-in these changes if you want? Mostly minor updates.

Copy link
Owner

@Illu Illu left a comment

Choose a reason for hiding this comment

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

Great work! Your changes are concise and the App works perfectly with them 😄

@Illu
Copy link
Owner

Illu commented Aug 4, 2020

About your previous comments:
Just in case, we can keep the "TDB" section in the calendar for now. LSP wiki pages link is fine, and using the abbreviation is perfect for smaller screens!
The notifications are most likely broken since they still use the wsstamp data, although simply replacing it with the new net without changing anything else should work. This is one the most annoying thing to test so we'll maybe take care of it in the next few days.

I feel like the dependencies (both Pods and JS) should be updated in another commit, as they aren't related to the API changes. I'll take care of it before releasing the next App version.

About the ResultCard test, it is because the component used to display the value as received from the API, and now the value is being used as an actual Date. If you want, you can change the "$_TEST_NET_DATE_$" to a dateTime format (ex: "2020-08-05T02:00:00Z" in the ResultCard.test.tsx file to fix it (you'll still need to update the snapshot with yarn test --update-snapshot).

This looks very good to me. If you want, you can update the failing test. Notifications are required to ship the next version but they can be taken care of later. Other than that, I can't wait to add your changes to Moonwalk! 😄

@derkweijers
Copy link
Contributor Author

I'll take a look at the test today, and figure out if I can fix the notifications! Testing notifications is a PITA, we basically need a few launches for it.

I don't know if Moonwalk is on TestFlight, but feel free to add me once testing starts 😃

@derkweijers
Copy link
Contributor Author

Done! Test is running smoothly again (really need to read into Jest...).

Notifications are also updated in e56be14. I'm not sure if the Android one will work, but I don't think you support Android yet?

I was wondering if the current notification flow will work anyway. Correct me if I'm wrong, but currently notifications will only be set/updated when the app is open. So any last minute changes (with yesterdays hop, for example) will most likely be missed if the user doesn't open the app.

Copy link
Owner

@Illu Illu left a comment

Choose a reason for hiding this comment

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

Awesome!
Android isn't supported, and it would require to re-write every component and screen to match the Android material design looks. I prefer focusing on offering a beautiful and familiar interface for iOS users right now.

The way notifications are handled is very basic, and last minute changes aren't taken into account. I think the only way to make this work would be to have a backend that send notifications to the users. Maybe in the next version 🙂

This looks good to me 👍 I'll merge this, update the notifications code and update the dependencies to prepare a release in the next few days

@Illu Illu merged commit 4651599 into Illu:master Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants