Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] My location button and my location fixes #1809

Closed
wants to merge 65 commits into from
Closed

[google_maps_flutter] My location button and my location fixes #1809

wants to merge 65 commits into from

Conversation

otopba
Copy link
Contributor

@otopba otopba commented Jul 6, 2019

Description

Fix some confusions with my location and my location button

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@otopba otopba requested review from amirh and iskakaushik as code owners July 6, 2019 15:01
@collinjackson collinjackson self-requested a review July 24, 2019 23:49
@otopba
Copy link
Contributor Author

otopba commented Jul 26, 2019

@collinjackson Hi!
Could you tell me, how can I fix this error?

  • ./CHANGELOG.md doesn't mention current version (0.5.20+1).

@iskakaushik
Copy link
Contributor

Thanks for the excellent contribution. If you can fix the merge conflicts we can merge this PR.

iskakaushik
iskakaushik approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

otopba added 3 commits August 7, 2019 02:39
@otopba
Merge pull request #2 from flutter/master
bb12418 
Update master
@otopba
Merge branch 'master' into my-location-button-fixes
ca8aa5e 
# Conflicts:
#	packages/google_maps_flutter/CHANGELOG.md
#	packages/google_maps_flutter/pubspec.yaml
@otopba
version: 0.5.21
45b417e
@otopba
Copy link
Contributor Author

otopba commented Aug 7, 2019

@iskakaushik Thank you. Done

@RyanRamchandar
Copy link

@otopba Did you get a chance to look at these conflicts so we can merge this?

otopba added 2 commits August 31, 2019 12:56
@otopba
Merge pull request #3 from flutter/master
a03ee88 
Update from master
@otopba
Merge branch 'master' into my-location-button-fixes
a809939 
# Conflicts:
#	AUTHORS
#	packages/google_maps_flutter/CHANGELOG.md
#	packages/google_maps_flutter/example/test_driver/google_maps.dart
@otopba
Copy link
Contributor Author

otopba commented Aug 31, 2019

@RyanRamchandar done

@otopba
Copy link
Contributor Author

otopba commented Sep 4, 2019

@iskakaushik
Hello! Could you merge this PR? Thank you

cyanglaz
cyanglaz reviewed Mar 18, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM! As the e2e test is blocked by permissions, is it possible to add unit tests to just test the platform code?

packages/google_maps_flutter/google_maps_flutter/example/test_driver/google_maps_e2e.dart Outdated
myLocationButtonEnabled = await inspector.isMyLocationButtonEnabled();
expect(myLocationButtonEnabled, false);
}, skip: true);
//TODO: Remove `skip' when https://github.com/flutter/flutter/issues/12561 will be fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only blocking android? Or iOS as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iOS will show a dialog with the permission request. So it is blocking UI

Copy link
Contributor

Choose a reason for hiding this comment

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

We enabled XCUITests in the repo, now we should be able to test this using XCUITests, XCUITests can handle permission. https://github.com/flutter/plugins/blob/master/CONTRIBUTING.md#setting-up-xcuitests

An example could be found here. #3254 (The tests in PR also handles permissions)
Do you mind adding the test?

@otopba
Copy link
Contributor Author

otopba commented Mar 22, 2020 •
edited
Loading

LGTM! As the e2e test is blocked by permissions, is it possible to add unit tests to just test the platform code?

There are already tests and they work well

@otopba otopba requested a review from cyanglaz March 22, 2020 19:19
@cyanglaz
Copy link
Contributor

There are already tests and they work well

Which tests did you refer to?

@iskakaushik iskakaushik removed their request for review April 28, 2020 16:35
otopba added 3 commits May 1, 2020 01:19
@otopba
Merge remote-tracking branch 'upstream/master'
0a86ebc
@otopba
Merge remote-tracking branch 'upstream/master'
6cd64da
@otopba
Merge branch 'master' into my-location-button-fixes
133f90e 
# Conflicts:
#	packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md
#	packages/google_maps_flutter/google_maps_flutter/pubspec.yaml
@otopba
Copy link
Contributor Author

otopba commented May 15, 2020

There are already tests and they work well

Which tests did you refer to?

google_maps_e2e.dart

@otopba
Copy link
Contributor Author

otopba commented Jul 24, 2020

@cyanglaz Any updates?

otopba and others added 7 commits August 20, 2020 10:37
@otopba otopba requested review from amirh and iskakaushik and removed request for collinjackson November 1, 2020 17:00
@otopba
Copy link
Contributor Author

otopba commented Nov 1, 2020

@cyanglaz friendly ping

@google-cla
Copy link

google-cla bot commented Nov 1, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 1, 2020
@cyanglaz
Copy link
Contributor

I'm going to close the PR because the CLA is not signed. Feel free to ping me when the CLA is signed and I'll re-open the PR. Or you can open a new PR after signing the CLA.

@cyanglaz cyanglaz closed this Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@amirh amirh Awaiting requested review from amirh

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

Assignees
No one assigned
Labels
cla: no
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@otopba @iskakaushik @RyanRamchandar @cyanglaz @amirh @googlebot @collinjackson @neokree