Skip to content

Traffic Plugin #6

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
Merged
tobrun merged 1 commit into from
Apr 11, 2017
Merged

Traffic Plugin #6

tobrun merged 1 commit into from
Apr 11, 2017

Conversation

@tobrun
Copy link
Member

@tobrun tobrun commented Apr 7, 2017 •
edited
Loading

ezgif com-video-to-gif 15

Closes #4

@tobrun tobrun added this to the v0.1 milestone Apr 7, 2017
@tobrun tobrun self-assigned this Apr 7, 2017
@tobrun tobrun requested review from ericrwolfe and zugaldia April 7, 2017 07:19
@tobrun
Copy link
Member Author

tobrun commented Apr 10, 2017 •
edited
Loading

@zugaldia I found the remaining issue (not having filters setup).
I did a cleanup and added javadoc. Ready for review.

@tobrun tobrun changed the title WIP Traffic Plugin Traffic Plugin Apr 10, 2017
ericrwolfe
ericrwolfe reviewed Apr 10, 2017
View reviewed changes
Copy link
Contributor

@ericrwolfe ericrwolfe left a comment

Choose a reason for hiding this comment

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

I'll leave the final approval up to @zugaldia, but at a high level everything looks great to me.

Traffic.java looks simple to use by itself and straightforward to customize should someone need to.

Should we update the repo's readme with installation instructions and a short code snippet around usage, or will this get out of hand as we add more plugins? Are there better ways to deal with this on Android?

@tobrun
Copy link
Member Author

tobrun commented Apr 11, 2017

Should we update the repo's readme with installation instructions and a short code snippet around usage, or will this get out of hand as we add more plugins? Are there better ways to deal with this on Android?

My suggestion would be to add README.md to each separate plugin. @zugaldia started created an overview README.md where we could link the different plugin pages.

@tobrun tobrun added the feature label Apr 11, 2017
zugaldia
zugaldia requested changes Apr 11, 2017
View reviewed changes
plugins/app/build.gradle Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Is this line necessary at all?

...app/src/main/java/com/mapbox/mapboxsdk/plugins/testapp/activity/FeatureOverviewActivity.java Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Let's simply remove it?

plugins/app/src/main/res/values/strings.xml Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Let's call it Navigation in preparation for #12.

plugins/app/src/main/res/values/strings.xml Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

nit: traffic

plugins/traffic/build.gradle Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Remove too?

plugins/traffic/src/main/java/com/mapbox/mapboxsdk/plugins/traffic/Traffic.java Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Nit: traffic

plugins/traffic/src/main/java/com/mapbox/mapboxsdk/plugins/traffic/Traffic.java Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Let's ticket for version 0.2?

plugins/traffic/src/main/java/com/mapbox/mapboxsdk/plugins/traffic/Traffic.java Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Let's ticket for version 0.2?

plugins/traffic/src/main/java/com/mapbox/mapboxsdk/plugins/traffic/Traffic.java Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Could we call this class TrafficPlugin instead? This could be the only convention for now, all plugins should have the Plugin suffix.

plugins/traffic/build.gradle Outdated
Copy link
Member

@zugaldia zugaldia Apr 11, 2017

Choose a reason for hiding this comment

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

Let's start with version 0.1 for now so that we aren't tied to SEMVER.

@zugaldia
Copy link
Member

zugaldia commented Apr 11, 2017

@tobrun Plugin is in great shape, I only added a few minor comments.

My suggestion would be to add README.md to each separate plugin. @zugaldia started created an overview README.md where we could link the different plugin pages.

Agreed. Let's start with the basics, which is to list all the plugins in the README file including Maven/Gradle installation instructions and a link to the examples in the Test App.

@langsmith will be running point for the next iteration of docs and include them in the website. I'll ticket that separately.

@tobrun
Copy link
Member Author

tobrun commented Apr 11, 2017

Rebased with master for PR with CI, review comments addressed and ready for review.

@tobrun tobrun merged commit 54ef518 into master Apr 11, 2017
@tobrun tobrun deleted the tvn-traffic branch April 11, 2017 15:24
@tobrun tobrun mentioned this pull request Apr 11, 2017
Closed
@tobrun
Copy link
Member Author

tobrun commented Apr 11, 2017

Follow up ticket to handle the todo comments found in this PR in #14.

@ivan-shen ivan-shen mentioned this pull request Nov 2, 2017
Closed
@psinetron psinetron mentioned this pull request Mar 6, 2018
Closed
@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

@zugaldia zugaldia zugaldia approved these changes

+1 more reviewer

@ericrwolfe ericrwolfe ericrwolfe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@tobrun tobrun

Labels

feature

Projects

None yet

Milestone

v0.1

Development

Successfully merging this pull request may close these issues.

3 participants

@tobrun @zugaldia @ericrwolfe