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

Issue #9: Improve Factory method #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marcelpinto
Copy link

Only accept the position of the marker so the onCreateAnnotation is
clearer and does not duplicate the work that should be done in
onBindAnnotation. That simplifies the adapter and avoid possible
duplication of work and resources.

Only accept the position of the marker so the onCreateAnnotation is
clearer and does not duplicate the work that should be done in
onBindAnnotation. That simplifies the adapter and avoid possible
duplication of work and resources.
@josh-burton
Copy link
Collaborator

Hi, thanks for the pull request.

I completely agree with you - it is a little confusing. I'll have a bit more of a think around onCreate vs onBind.

One thing I will say in the meantime is that we'll need to maintain backwards compatibility, so rather than removing methods from the annotation factory, can you deprecate them instead?

@marcelpinto
Copy link
Author

Ok I thought that you were still under beta so no need of backward compatibility. I will try to refactor it.

Actually, I have some doubts on the need of the AnnotationFactory to create markers. Why not let the user create the markers without need of the factory.

Another point is that MapAnnotation exposes addToMap, remove and annotate, this might lead to misusage of it.

@josh-burton
Copy link
Collaborator

The factory is there as an abstraction on top of the underlying map. By using the factory, it makes it really easy to swap out the map provider by just changing the parent adapter class.

Good point about those public methods though!

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

Successfully merging this pull request may close these issues.

2 participants