Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Magic Window Demo #886

Merged
merged 13 commits into from
Oct 16, 2018
Merged

Magic Window Demo #886

merged 13 commits into from
Oct 16, 2018

Conversation

sansumbrella
Copy link
Contributor

Adds magic window demo showing two maps kept in sync and alpha masking with the Android view system.

screenshot_1539370397
screenshot_1539370403
screenshot_1539370425

Closes #613

David Wicks added 5 commits October 12, 2018 13:08
@sansumbrella
Copy link
Contributor Author

sansumbrella commented Oct 12, 2018

Questions for reviewers

What kinds of tests do we typically add for examples?
Class organization? All classes are currently in one file to keep the demo together.

  • naming: I noticed the other kotlin demo currently has Kotlin in the file/classname

What linter do you run locally (to avoid back-and-forth with circleci)?

autoformat will be nice...
@langsmith
Copy link
Contributor

ezgif com-resize

@langsmith
Copy link
Contributor

Hell of an example @sansumbrella . 👏 🎊 Made a commit that cleans up some small stuff; mainly moving string resource files to their correct locations.

const val PERMISSION_REQUEST_LOCATION = 404
const val BASE_MAP_BUNDLE = "$TAG.basemap.bundle"
const val REVEAL_MAP_BUNDLE = "$TAG.revealedMap.bundle"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add javadocs to explain what this example is about. E.g:

/**
 * This example...
 */

const val TAG = "DragActivityTag"
const val PERMISSION_REQUEST_LOCATION = 404
const val BASE_MAP_BUNDLE = "$TAG.basemap.bundle"
const val REVEAL_MAP_BUNDLE = "$TAG.revealedMap.bundle"
Copy link
Contributor

Choose a reason for hiding this comment

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

For these const val, please move them within the activity below the var initialZoom line with

companion object {
        const val TAG = "DragActivityTag"
        const val PERMISSION_REQUEST_LOCATION = 404
        const val BASE_MAP_BUNDLE = "$TAG.basemap.bundle"
        const val REVEAL_MAP_BUNDLE = "$TAG.revealedMap.bundle"
}

const val BASE_MAP_BUNDLE = "$TAG.basemap.bundle"
const val REVEAL_MAP_BUNDLE = "$TAG.revealedMap.bundle"

class MagicWindowActivity : AppCompatActivity(), LocationEngineListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's go ahead and throw the word Kotlin into the example's name. MagicWindowKotlinActivity?

@langsmith
Copy link
Contributor

What kinds of tests do we typically add for examples?

Not much. We run a test that makes sure that the example can be opened and doesn't crash:

We also do Firebase instrumentation tests

Class organization? All classes are currently in one file to keep the demo together.

I'm not sure what the question/concern here is. A little clarification?

naming: I noticed the other kotlin demo currently has Kotlin in the file/classname

Yes, let's have Kotlin examples have the word Kotlin in their names.

What linter do you run locally (to avoid back-and-forth with circleci)?

We don't? Don't think we do. Running make checkstyle in Terminal will check for checkstyle issues. Was there a specific reason why you were asking about this?

@sansumbrella
Copy link
Contributor Author

sansumbrella commented Oct 15, 2018

Thanks for the feedback (and cleanup)! Getting on that now.

My class organization question was just about whether we are happy to keep all code for one demo in a single file or if we want to have a single file per class. For demos, I think it makes sense to keep all the code in a single file as long as it isn't too big. I think other demos in this repo are organized that way.

Running make checkstyle in Terminal will check for checkstyle issues.

This will do the trick. This sample just failed tests for a while since I wasn't running that locally. It would be cool if there were editor integration that auto-formatted to the right style a la gofmt or rustfmt. Maybe someday.

@sansumbrella
Copy link
Contributor Author

Should be ready for another round of review and merging! Thanks.

@langsmith
Copy link
Contributor

keep all code for one demo in a single file

For now, let's do this. So keep things as is.

option + command + l in Android Studio does autoformatting, which helps for checkstyle

@sansumbrella sansumbrella merged commit d0e61bc into master Oct 16, 2018
@sansumbrella sansumbrella deleted the dw-magic-window branch October 16, 2018 17:29
@langsmith langsmith mentioned this pull request Nov 1, 2018
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants