Shorten Firebase Dynamic Links #719
Conversation
| import retrofit2.http.Query; | ||
|
|
||
| /** | ||
| * Created by unstablebrainiac on 26/5/17. |
There was a problem hiding this comment.
Please remove the comment. 😄 thanks
|
|
||
| @FormUrlEncoded | ||
| @POST("/v1/shortLinks") | ||
| Call<FirebaseDynamicLinksResponse> getShortenedUrl(@Query("key") String api_key, @Body RequestBody body); |
There was a problem hiding this comment.
Just naming: Can you rename api_key to apiKey. We try not to use snail case.
Also what about shortenUrl as a method name?
| * Created by unstablebrainiac on 26/5/17. | ||
| */ | ||
|
|
||
| public final class FirebaseDynamicLinksHubFactory { |
There was a problem hiding this comment.
Hub is the name for the api for GDG-X. Do we need Hub naming here? I would prefer FirebaseDynamicLinksFactory
| * Created by unstablebrainiac on 26/5/17. | ||
| */ | ||
|
|
||
| public interface FirebaseDynamicLinksHub { |
There was a problem hiding this comment.
What about just FirebaseDynamicLinks
| */ | ||
|
|
||
| public class FirebaseDynamicLinksResponse { | ||
| private String shortLink; |
There was a problem hiding this comment.
Please mark the fields final. With this you need to remove the setter methods.
| @Override | ||
| public void onSuccess(FirebaseDynamicLinksResponse response) { | ||
| createChooser(HttpUrl.parse(response.getShortLink())); | ||
| shareProgressDialog.dismiss(); |
There was a problem hiding this comment.
Let's try to decouple the UI concerns and the actual link shortening.
Now the link creation is asynchronous. Ideally we should have a Listener with a possible method onDynamicLinkReady(String url). Listener's method would be called with the shortened link when it is successful.
Then the place where we implement this Listener can handle progress show/dismiss.
What do you think?
There was a problem hiding this comment.
Resolved all the other errors but did not understand this very well.
Isn't the Retrofit callback the method that you're talking about? Do you want me to create a separate method and call it from within onSuccess(FirebaseDynamicLinksResponse response)? If yes, how do you propose to show the ProgressDialog before making the request?
Also, is there anything similar in the codebase that'll probably give me a better idea?
There was a problem hiding this comment.
Unfortunately not. The app doesn't follow any architecture and async handling codes are really old. We didn't have the chance to refactor yet.
Would you be up to pair on this? We can connect via ScreenHero and do it together.
There was a problem hiding this comment.
Sure thing. But Screenhero is not accepting new signups right now so you'll have to send me an invite. My email ID is [email protected] and I am UnstableBrainiac on Slack.
There was a problem hiding this comment.
Hey! Sorry for the delayed response. I've been trying to get Screenhero to work on my system but couldn't. I even re-installed Windows to get it working, but in vain.
I can still contribute though. Let me know what you need me to do.
There was a problem hiding this comment.
I didn't have time to check the latest version of the PR. Sorry.
Let's schedule some time and talk about it. Can also be a chat or Hangouts video call.
There was a problem hiding this comment.
I will review once more and get back to you.
| @@ -63,4 +78,30 @@ private String createDeepLink(String gplusId) { | |||
| .toString(); | |||
| } | |||
|
|
|||
| private void shareShortAppInviteLink(String longUrl) { | |||
| FirebaseDynamicLinksHub firebaseDynamicLinksHub = App.from(activity).getFirebaseDynamicLinksHub(); | |||
| String requestString = "{\"longDynamicLink\": \"" + longUrl + "\",\"suffix\": {\"option\": \"SHORT\"}}"; | |||
There was a problem hiding this comment.
Just like the response, can you create a Request class with these fields?
Then Retrofit and Gson will automatically create the json body for you. This will be much cleaner.
app/build.gradle
Outdated
| @@ -66,6 +66,7 @@ android { | |||
|
|
|||
| buildConfigField "String", "IP_SIMPLE_API_ACCESS_KEY", "\"$local_properties.ip_simple_api_access_key\"" | |||
| buildConfigField "String", "ANDROID_SIMPLE_API_ACCESS_KEY", "\"$local_properties.android_simple_api_access_key\"" | |||
| buildConfigField "String", "FIREBASE_WEB_API_KEY", "\"$local_properties.firebase_web_api_key\"" | |||
There was a problem hiding this comment.
Are you sure that you need a new api key? I think IP_SIMPLE_API_ACCESS_KEY would work. If you don't have that key, simple create one in Google api console. If you're not sure, feel free to ask us at https://gitter.im/gdg-x/frisbee
|
Can you also put screen recording or screenshot (with the progress dialog) after you addressed the comments. Thanks. |
|
Here is a screencast of the app. I have addressed all the comments. Let me know if there are any more changes to be made. |
|
Sorry that I still didn't review the code. By the time, Google released a new apis to do this in an easier way. Would you be OK to check that out and apply? I think we can also merge this PR and iterate. What do you think? https://firebase.googleblog.com/2017/06/making-dynamic-links-easier.html |
|
You can merge this PR and create a separate issue for the new APIs |
|
@unstablebrainiac sorry that your PR still not merged. You have problems with checkstyle. Can you look at the report from Travis and fix little problems with the code. |
Fixes #708