Skip to content

Add web support to place_tracker #550

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

Merged
merged 15 commits into from
Sep 23, 2020
Merged

Add web support to place_tracker #550

merged 15 commits into from
Sep 23, 2020

Conversation

johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Sep 22, 2020

This adds web support to place_tracker. (google_maps_flutter_web was added in flutter/plugins#2933 and is on pub now.)

To use build/test for web, you still need to be on the beta, dev, or master channel, but I don't think we need to move into the experimental/ directory in this case, since this app still works on the stable channel on iOS and Android.

FYI @ditman

Copy link
Member

@ditman ditman 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 awesome! Thanks for publishing this!

Copy link
Contributor

@RedBrogdon RedBrogdon left a comment

Choose a reason for hiding this comment

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

LGTM other than one nit and one question. 😄

@@ -69,3 +69,6 @@
!**/ios/**/default.pbxuser
!**/ios/**/default.perspectivev3
!/packages/flutter_tools/test/data/dart_dependencies_test/**/.packages

# Flutter Web files
lib/generated_plugin_registrant.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: restore newline.

@@ -414,11 +429,13 @@ class PlaceMapState extends State<PlaceMap> {
switch (category) {
case PlaceCategory.favorite:
return BitmapDescriptor.fromAssetImage(
createLocalImageConfiguration(context), 'assets/heart.png');
createLocalImageConfiguration(context, size: Size.square(32)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll miss those enormous blue flags. 😄


<!-- // Other stuff -->

<script src="https://maps.googleapis.com/maps/api/js?key=AIzaSyBEKgpAdjMVMv9IKX-BBsjzwgYMxfZonbw"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the key intended to be in here? We don't ship the app with the mobile API keys, which is why I'm asking. Even if this one isn't as "secret" as the mobile versions, it might be better to disassociate the app from any one account and update the README to tell people where to stick their own key.

Copy link
Contributor Author

@johnpryan johnpryan Sep 23, 2020

Choose a reason for hiding this comment

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

Normally I would do that, but If we take it out, we can't deploy it to http://flutter.github.io/samples anymore.

We could consider writing this string as part of the samples index deployment (perhaps via an environment variable given to a GitHub action.) but the API key is still visible if you view the source of the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a section to the README for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I added some domain restrictions:

Screen Shot 2020-09-23 at 11 05 43 AM

Copy link
Member

Choose a reason for hiding this comment

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

Deleted my previous comment because I just stated the obvious (again) xD

@johnpryan johnpryan merged commit 2236f12 into flutter:master Sep 23, 2020
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.

3 participants