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.

Sign up for GitHub

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

Jump to bottom

Building plugin #26

Merged
merged 5 commits into from
Jul 18, 2017
Merged

Building plugin #26

merged 5 commits into from
Jul 18, 2017

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Apr 28, 2017

This PR adds a Building plugin, this plugin API matches Traffic plugin API.

westminster abbey

@tobrun tobrun added the feature label Apr 28, 2017
@tobrun tobrun self-assigned this Apr 28, 2017
@tobrun tobrun mentioned this pull request May 3, 2017
Closed
@tobrun
Copy link
Member Author

tobrun commented May 10, 2017

I finished the initial stab and this PR is ready for review.

Possible API to add:

  • color?
  • opacity?

@tobrun tobrun requested a review from zugaldia May 10, 2017 11:27
@tobrun tobrun changed the title WIP add Building plugin Building plugin May 10, 2017
@zugaldia
Copy link
Member

@tobrun Agreed. I think an API for the three different values you set on https://github.com/mapbox/mapbox-plugins-android/pull/26/files#diff-a1a6e9c8cc422ca880b018bba0a5741cR101 (minZoom, color, opacity) is all we need for now.

@tobrun tobrun force-pushed the tvn-building-plugin branch 2 times, most recently from 2f63028 to f994a2c Compare May 15, 2017 12:36
@tobrun
Copy link
Member Author

tobrun commented May 15, 2017 •
edited
Loading

I have added the API and reworked the code to reflect this. This PR is ready for review, though we should maybe wait with merging until v5.1.0 final is released?

@zugaldia
Copy link
Member

This PR is ready for review, though we should maybe wait with merging until v5.1.0 final is released?

👍 Makes sense to me to wait until the underlying API gets stable and published to release 0.1.0. Meanwhile, I suggest we start publishing SNAPSHOTs of the plugin so that interested devs can start playing with it.

@tobrun
Copy link
Member Author

tobrun commented Jul 5, 2017

This PR has been updated with latest SDK release and adds Light integration (just a setter for now)

@cammace
Copy link
Contributor

cammace commented Jul 6, 2017

Duplicate import to line 8 - timber.log.Timber. [RedundantImport]

Checkstyle strikes again 😆

cammace
cammace reviewed Jul 6, 2017
View reviewed changes
plugins/app/src/main/java/com/mapbox/mapboxsdk/plugins/testapp/activity/BuildingActivity.java Outdated
@BindView(R.id.fabBuilding)
View fab;

@BindView(R.id.seekbar_light_radialCoordinate)
Copy link
Contributor

Choose a reason for hiding this comment

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

half resources use camel casing, the rest use snake

plugins/building/gradle.properties Outdated
@@ -0,0 +1,4 @@
POM_ARTIFACT_ID=mapbox-android-building-plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep consistency around plugins, the artifact id should be mapbox-android-plugin-building

plugins/building/gradle.properties Outdated
@@ -0,0 +1,4 @@
POM_ARTIFACT_ID=mapbox-android-building-plugin
POM_NAME=Mapbox Android Plugins
POM_DESCRIPTION=Mapbox Android Plugins (Building)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mapbox Android Building Plugin

plugins/building/proguard-rules.pro Outdated
@@ -0,0 +1,25 @@
# Add project specific ProGuard rules here.
Copy link
Contributor

Choose a reason for hiding this comment

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

file can be deleted?

@cammace
Copy link
Contributor

cammace commented Jul 8, 2017

Went ahead and fixed the issues I raised previously. One thing I noticed is that we use setVisibility(boolean visible) in this plugin but in traffic we have toggle(). Since they essentially do the same thing, we should make the APIs the same (I prefer the setVisibility API).

Another addition that should be made in this PR would be the zoom function modifies the building height so they don't just pop in.

@cammace
Copy link
Contributor

cammace commented Jul 11, 2017

If this is supported across different styles, we should make sure everything is being redrawn correctly when styles change.

zugaldia
zugaldia requested changes Jul 11, 2017
View reviewed changes
Copy link
Member

@zugaldia zugaldia left a comment

Choose a reason for hiding this comment

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

Very minor feedback.

plugins/build.gradle Outdated
@@ -10,6 +10,7 @@ buildscript {
allprojects {
repositories {
jcenter()
maven { url "http://oss.sonatype.org/content/repositories/snapshots/" }
Copy link
Member

Choose a reason for hiding this comment

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

What's this needed for?

Copy link
Contributor

Choose a reason for hiding this comment

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

leftover from when we were using map SDK snapshot I think. I removed it.

plugins/app/src/main/java/com/mapbox/mapboxsdk/plugins/testapp/activity/BuildingActivity.java Outdated
}

private void initLightSeekbar() {
seekbarRadialCoordinate.setMax(24); // unknown?
Copy link
Member

Choose a reason for hiding this comment

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

What does unknown mean in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tobrun could speak more on this, I'm not sure either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the maximum radius is for a light source, refs https://en.wikipedia.org/wiki/Spherical_coordinate_system

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a note on code mentioning this?

plugins/app/src/main/java/com/mapbox/mapboxsdk/plugins/testapp/activity/BuildingActivity.java Outdated

@Override
public void onStartTrackingTouch(SeekBar seekBar) {

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment or a Timber call in the unused methods to explicitly note they aren't used on purpose?

@tobrun
# This is a combination of 5 commits.
a689a33 
# This is the 1st commit message:
add Building plugin

# The commit message #2 will be skipped:

#	fixed issues reviewed by me

# The commit message #3 will be skipped:

#	fixed bitrise script

# The commit message #4 will be skipped:

#	added DDS for building height

# The commit message #5 will be skipped:

#	removed commented out code
@zugaldia zugaldia modified the milestone: v0.2 Jul 17, 2017
zugaldia
zugaldia approved these changes Jul 18, 2017
View reviewed changes
Copy link
Member

@zugaldia zugaldia left a comment

Choose a reason for hiding this comment

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

Let's 🏛 this.

plugins/app/src/main/java/com/mapbox/mapboxsdk/plugins/testapp/activity/BuildingActivity.java Outdated
}

private void initLightSeekbar() {
seekbarRadialCoordinate.setMax(24); // unknown?
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a note on code mentioning this?

@cammace cammace merged commit c39ff12 into master Jul 18, 2017
@cammace cammace deleted the tvn-building-plugin branch July 18, 2017 23:52
@MArman88 MArman88 mentioned this pull request Sep 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cammace cammace cammace left review comments

@zugaldia zugaldia zugaldia approved these changes

Assignees

@tobrun tobrun

Labels
feature
Projects
None yet
Milestone
v0.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tobrun @zugaldia @cammace