Firebase dynamic links #563

bparrishMines · 2018-05-10T21:25:37Z

Initial release for Firebase Dynamic Links

…amic_links

mravn-google

It's getting late here. I'll continue tomorrow.

mravn-google · 2018-05-16T08:49:14Z

packages/firebase_dynamic_links/example/ios/Podfile

@@ -0,0 +1,63 @@
+# Uncomment this line to define a global platform for your project


Podfiles should not be included in the repo. Flutter tools autogenerates those, if missing.

This one is out of date, by the way.

mravn-google · 2018-05-22T20:10:04Z

packages/firebase_dynamic_links/CHANGELOG.md

@@ -0,0 +1,3 @@
+## 0.0.1
+
+* Initial Release


[nit] Change log entries should be grammatically complete phrases using standard US spelling. So

* Initial release.

would be more correct (though it arguably still conveys too little information; what are the initial features?).

mravn-google · 2018-05-22T20:10:50Z

packages/firebase_dynamic_links/README.md

+
+*Note*: This plugin is still under development, and some APIs might not be available yet. [Feedback](https://github.com/flutter/flutter/issues) and [Pull Requests](https://github.com/flutter/plugins/pulls) are most welcome!
+
+## Usage


[nit] Please add empty line after the heading for consistency with the other sections.

mravn-google · 2018-05-22T20:13:40Z

packages/firebase_dynamic_links/README.md

+You can create short or long Dynamic Links with the Firebase Dynamic Links Builder API. This API accepts either a long Dynamic Link or an object containing Dynamic Link parameters, and returns a URL like the following example:
+
+```dart
+


[nit] Padding lines at the start and end of code snippets should not be necessary. The md2html transformation should already take care of it in a visually pleasing way.

Oh, and this snippet is not Dart by the way.

mravn-google · 2018-05-22T20:14:45Z

packages/firebase_dynamic_links/README.md

+```dart
+
+final DynamicLinkComponents components = new DynamicLinkComponents(
+    domain: 'abc123.app.goo.gl',


[nit] Dart indentation is 2 spaces. This code uses both 4 and 2 spaces.

mravn-google · 2018-05-22T21:56:01Z

packages/firebase_dynamic_links/pubspec.yaml

+
+environment:
+  sdk: ">=2.0.0-dev.28.0 <3.0.0"
+  flutter: ">=0.2.4 <2.0.0"


Please end file with an empty line.

mravn-google · 2018-05-22T22:02:36Z

packages/firebase_dynamic_links/lib/src/short_dynamic_link.dart

+  /// Short url value.
+  final Uri shortUrl;
+
+  /// Gets information about potential warnings on link creation.


Getters should be documented with a noun phrase describing what the property is.

https://www.dartlang.org/guides/language/effective-dart/documentation#prefer-starting-variable-getter-or-setter-comments-with-noun-phrases

mravn-google · 2018-05-22T22:06:11Z

packages/firebase_dynamic_links/lib/src/navigation_info_parameters.dart

+class NavigationInfoParameters {
+  NavigationInfoParameters({this.forcedRedirectEnabled});
+
+  /// Should forced non-interactive redirect be used when link is tapped on mobile device.


This is formulated like a question, but no question mark? Maybe

/// Whether forced non-interactive redirect should be used when link is tapped on mobile device.

mravn-google · 2018-05-22T22:12:58Z

packages/firebase_dynamic_links/lib/src/dynamic_link_components.dart

+/// a domain and a randomized path. Long URLs will have a domain and a query
+/// that contains all of the Dynamic Link parameters.
+class DynamicLinkComponents {
+  DynamicLinkComponents({


Dartdoc says this class is used for Dynamic Link URL generation. Dartdoc also implies that instances somehow describe Dynamic Link parameters. The class is called DynamicLinkComponents.

This is mildly confusing terminology...

mravn-google · 2018-05-22T22:15:13Z

packages/firebase_dynamic_links/lib/src/dynamic_link_components.dart

+  });
+
+  /// Applies Android parameters to a generated Dynamic Link URL.
+  AndroidParameters androidParameters;


Please make this class immutable, if at all possible. It seems clients are supposed to specify all components using the constructor. And all the parameter classes below are immutable.

mravn-google

I'm done. Some more nits and a few real change requests. Overall, LVGTM.

mravn-google · 2018-05-25T20:27:12Z

packages/firebase_dynamic_links/.gitignore

@@ -0,0 +1,8 @@
+.DS_Store


Please remove all .gitignore files from individual plugins. If you rebase to master, the repo root .gitignore should make git ignore all the files we don't want to check in.

mravn-google · 2018-05-25T20:28:33Z

packages/firebase_dynamic_links/android/build.gradle

+        disable 'InvalidPackage'
+    }
+    dependencies {
+        api 'com.google.firebase:firebase-invites:15.0.0'


Please update this dependency to 15.+ so it stays consistent with other firebase plugins.

mravn-google · 2018-05-25T20:38:37Z

...ndroid/src/main/java/io/flutter/plugins/firebasedynamiclinks/FirebaseDynamicLinksPlugin.java

+      String content = (String) googleAnalyticsParameters.get("content");
+      String medium = (String) googleAnalyticsParameters.get("medium");
+      String source = (String) googleAnalyticsParameters.get("source");
+      String term = (String) googleAnalyticsParameters.get("term");


You could consider adding a local helper method a la MethodCall.argument(String)

static <T> T valueFor(String key, Map<String, Object> map) { @SuppressWarnings("unchecked") T result = (T) map.get(key); return result; }

Then you can avoid all the casts.

mravn-google · 2018-05-25T20:42:24Z

packages/firebase_dynamic_links/example/android/app/gradle/wrapper/gradle-wrapper.properties

@@ -0,0 +1,6 @@
+#Mon May 07 13:02:14 PDT 2018


It appears you have removed the wrong gradle-wrapper.properties file. You want such a file in the example/android folder, not the app/ subfolder. And it should refer to gradle-4.1-all to be consistent with all the other plugins.

mravn-google · 2018-05-25T20:49:24Z

packages/firebase_dynamic_links/lib/firebase_dynamic_links.dart

+part 'src/itunes_connect_analytics_parameters.dart';
+part 'src/navigation_info_parameters.dart';
+part 'src/short_dynamic_link.dart';
+part 'src/social_meta_tag_parameters.dart';


I have not seen this "one part = one file = one class" style before with Dart. Seems like a Java thing, really. But I have no strong opinion here.

mravn-google · 2018-05-25T20:54:39Z

packages/firebase_dynamic_links/lib/src/dynamic_link_parameters.dart

+    @required this.link,
+    this.navigationInfoParameters,
+    this.socialMetaTagParameters,
+  });


You should really add }) : assert(domain != null), assert(link != null); here. The @required annotation yields an analyzer error, if you leave out the parameter at a call site, not if you explicitly set it to null.

Same in other classes.

mravn-google · 2018-05-25T20:56:28Z

packages/firebase_dynamic_links/test/firebase_dynamic_links_test.dart

+                  'title': 'bro',
+                },
+              }),
+        ]);


A few trailing commas (after ] I believe) should help reduce all indentation to two spaces in the above code.

mravn-google · 2018-05-25T20:59:01Z

packages/firebase_dynamic_links/test/firebase_dynamic_links_test.dart

@@ -0,0 +1,415 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.


New files should use the current year. (When update existing files, we leave the header as is.)

Applies in many places.

…amic_links

* Initial project creation * Added Firebase Dynamic Links library * Dart side for create dynamic links * Added gradle to .gitignore * Implement android create dynamic link * Removed gradle stuff * iOS implementation of creating link * Dynamic link creation fixes * Added short link creation * Added shortening of dynamic link * Created example UI * Added testing * License and plugin info * Added pub info * Brackets over braces * Comments and FLT prefix * Cleaned up platform code * Formatting changes * Added short link error exception * Formatting * More Formatting * Parentheses and fixed tests * Fixed README * Added short link warnings * Formatting * Formatting * More comments * Added ShortDynamicPathLength test * ShortLinks now throw flutter error * Formatting * Addressing some comments * Addressing more comments * Addressing more comments * Formatting * PR comments * Formatting and PR comments * Changed name Components -> Parameters * Updated README * Now I understand why trailing commas * Removed gradle-wrapper files * Removed .gitigores * Added map helper function and updated gradle * Added gradle-wrapper.properties * Added assert to test for nulls * Comma formatting * Update Licenses to 2018 * Formatting * Remove gradle properties

bparrishMines added 19 commits April 27, 2018 09:50

Initial project creation

4ca5190

Added Firebase Dynamic Links library

c0db0e5

Merge branch 'master' of github.com:flutter/plugins into firebase_dyn…

5881d09

…amic_links

Dart side for create dynamic links

b25013e

Added gradle to .gitignore

adc9d3b

Implement android create dynamic link

77d50ef

Removed gradle stuff

790e4ea

iOS implementation of creating link

99604ad

Dynamic link creation fixes

d409c01

Added short link creation

18160f5

Added shortening of dynamic link

06478d3

Created example UI

87d6a0b

Added testing

c78f9af

License and plugin info

4117b8c

Merge branch 'master' of github.com:flutter/plugins into firebase_dyn…

b281d6e

…amic_links

Added pub info

9b4d226

Brackets over braces

ab1dfc1

Comments and FLT prefix

ba4ec6e

Cleaned up platform code

d134f6e

googlebot added the cla: yes label May 10, 2018

bparrishMines added 6 commits May 10, 2018 15:31

Formatting changes

bf48dbc

Added short link error exception

8afac09

Formatting

16a751f

More Formatting

0352309

Parentheses and fixed tests

9d179ae

Fixed README

1528c93

bparrishMines requested a review from collinjackson May 11, 2018 21:07

collinjackson added the flutterfire label May 15, 2018

bparrishMines added 2 commits May 15, 2018 15:03

Added short link warnings

682dd4b

Merge branch 'master' of github.com:flutter/plugins into firebase_dyn…

db8c871

…amic_links

bparrishMines added 2 commits May 15, 2018 16:16

More comments

1621650

Added ShortDynamicPathLength test

ed1b8fd

collinjackson self-assigned this May 21, 2018

bparrishMines added 3 commits May 21, 2018 16:06

Merge branch 'master' of github.com:flutter/plugins into firebase_dyn…

39c1deb

…amic_links

ShortLinks now throw flutter error

ffabd9f

Formatting

736dd21

mravn-google reviewed May 22, 2018

View reviewed changes

bparrishMines added 10 commits May 23, 2018 10:19

Addressing some comments

309502b

Addressing more comments

b6b3adb

Addressing more comments

710ef5a

Formatting

4cbabb6

PR comments

fcd7dbe

Formatting and PR comments

a6805bd

Changed name Components -> Parameters

84956c2

Updated README

90f9625

Now I understand why trailing commas

c60bde0

Removed gradle-wrapper files

94ff409

mravn-google approved these changes May 25, 2018

View reviewed changes

bparrishMines added 9 commits May 29, 2018 10:33

Merge branch 'master' of github.com:flutter/plugins into firebase_dyn…

856865f

…amic_links

Removed .gitigores

f16c7ff

Added map helper function and updated gradle

438649b

Added gradle-wrapper.properties

bb1a015

Added assert to test for nulls

3126181

Comma formatting

9b6089b

Update Licenses to 2018

e2fa089

Formatting

8fdaf2e

Remove gradle properties

4535278

bparrishMines merged commit 5b8bcca into flutter:master May 29, 2018

bparrishMines deleted the firebase_dynamic_links branch May 29, 2018 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firebase dynamic links #563

Firebase dynamic links #563

bparrishMines commented May 10, 2018 •

edited

Loading

mravn-google left a comment

mravn-google May 16, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google May 22, 2018

mravn-google left a comment

mravn-google May 25, 2018

mravn-google May 25, 2018

mravn-google May 25, 2018

mravn-google May 25, 2018

mravn-google May 25, 2018

mravn-google May 25, 2018

mravn-google May 25, 2018

mravn-google May 25, 2018

		@@ -0,0 +1,63 @@
		# Uncomment this line to define a global platform for your project


		Note: This plugin is still under development, and some APIs might not be available yet. [Feedback](https://github.com/flutter/flutter/issues) and [Pull Requests](https://github.com/flutter/plugins/pulls) are most welcome!

		## Usage

		You can create short or long Dynamic Links with the Firebase Dynamic Links Builder API. This API accepts either a long Dynamic Link or an object containing Dynamic Link parameters, and returns a URL like the following example:

		```dart

		@@ -0,0 +1,415 @@
		// Copyright 2017 The Chromium Authors. All rights reserved.

Firebase dynamic links #563

Firebase dynamic links #563

Conversation

bparrishMines commented May 10, 2018 • edited Loading

mravn-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mravn-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparrishMines commented May 10, 2018 •

edited

Loading