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

[Feature] Add button to switch map type #406

Merged
merged 21 commits into from Mar 30, 2020

Conversation

shobhitagarwal1612
Copy link
Member

Demo:

map_type_2

Invert if conditions for simplification
@gino-m
Copy link
Collaborator

gino-m commented Mar 20, 2020

Thanks for the change! Is there any background on why this is needed or preferred? Before starting a change please make sure there's an Issue filed so we can discuss, since we need to consider all the different ongoing feature development and how they'll fit together.

Note that we're using the [] before the PRs and Issues to indicate which feature/subarea of the app is being changes, not the type of PR/issue.

@gino-m gino-m changed the title [Feature] Add button to switch map type [Mao] Add button to switch map type Mar 20, 2020
@gino-m gino-m changed the title [Mao] Add button to switch map type [Map] Add button to switch map type Mar 20, 2020
@shobhitagarwal1612 shobhitagarwal1612 changed the title [Map] Add button to switch map type [Feature] Add button to switch map type Mar 20, 2020
@shobhitagarwal1612
Copy link
Member Author

shobhitagarwal1612 commented Mar 20, 2020

Thanks for the change! Is there any background on why this is needed or preferred? Before starting a change please make sure there's an Issue filed so we can discuss, since we need to consider all the different ongoing feature development and how they'll fit together.

Sorry for being so impatient. I remember asking for this feature in hangouts some days ago and you replied that it would be somewhat similar to Google Maps. Point noted for future issues.

Points regarding use cases:

  • Satellite view requires better internet connection if offline tiles are not available as it consumes for data
  • I prefer using normal view when navigating using maps as it contains less details.

Note that we're using the [] before the PRs and Issues to indicate which feature/subarea of the app is being changes, not the type of PR/issue.

Done

@gino-m
Copy link
Collaborator

gino-m commented Mar 20, 2020

Sorry for being so impatient. I remember asking for this feature in hangouts some days ago and you replied that it would somewhat similar to Google Maps. Point noted for future issues.

No worries. We'd normally want to discuss such a change more in depth with UX Designers (@coreyleamon fyi), but since we know we'll need to have a layer switcher eventually to control offline imagery let's keep this change and iterate on it as we get more clarity. Over to @scolsen for review!

@shobhitagarwal1612 shobhitagarwal1612 added the type: fr Request for new feature label Mar 20, 2020
Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

Lgtm, just a few final changes.

scolsen
scolsen previously approved these changes Mar 26, 2020
Copy link
Contributor

@scolsen scolsen left a comment

Choose a reason for hiding this comment

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

Left a few comments as FYIs but otherwise LGTM

gnd/src/main/res/drawable/map_layers.xml Outdated Show resolved Hide resolved
gnd/src/main/res/layout/map_container_frag.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

Some small modifications, overall LGTM!

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the change!

@gino-m gino-m merged commit 325491c into google:master Mar 30, 2020
@shobhitagarwal1612 shobhitagarwal1612 deleted the map-type branch March 30, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes type: fr Request for new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants