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

Convert Camera to ES6 class syntax and inherit Map from it #3421

Merged
merged 1 commit into from
Oct 21, 2016

Conversation

mourner
Copy link
Member

@mourner mourner commented Oct 20, 2016

I was trying to make Camera a an object used from Map, as discussed in #3408, but it got really messy, with either lots of code duplication or splitting similar responsibilities between Map and Camera, and it also required rewriting tons of camera tests.

So I'm proposing the make Camera the base class of Map instead. This way it looks much cleaner, is a clear improvement over the current state (mixing in Camera.prototype), and doesn't require as many changes. We can follow up on any improvements in subsequent PRs (specifically, balancing out responsibilities between Camera, Transform and Map, and camera event refactoring #3357).

In addition to the change above, I made some minor cleanups in Camera code and also moved all camera events there. will follow up in another PR.

👀 @lucaswoj @jfirebaugh

@mourner
Copy link
Member Author

mourner commented Oct 21, 2016

@jfirebaugh per discussion in chat, rebased the PR to only contain the Camera ES6 class changes so that the diff is easy to read. Will follow-up with other changes in a different PR. Note that I added @memberof Map to all Camera methods so that it still shows up under Map while being inherited (didn't test it though since #3416 needs to be fixed first).

@mourner mourner merged commit 79e40c9 into master Oct 21, 2016
@mourner mourner deleted the compose-camera branch October 21, 2016 17:23
@mourner
Copy link
Member Author

mourner commented Oct 21, 2016

John approved the PR but GH was having issues; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant