Skip to content

Add ol.control.Control#setTarget #3139

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

Merged
merged 1 commit into from
Jan 16, 2015
Merged

Add ol.control.Control#setTarget #3139

merged 1 commit into from
Jan 16, 2015

Conversation

elemoine
Copy link
Member

This commit adds a setTarget method to ol.control.Control. This function can be used in cases where the target cannot be set at control construction time.

For example, with Angular, it makes sense to create the control instance in an Angular controller, and have a "control" directive insert the control into the DOM.


/**
* This function is used to set a target element for the control. This
* function will have no effect if it is called after the control has
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a limitation which is not necessary. With a little more code, you could use setTarget to move the control's markup around in the document. Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually gets much more complex if you go that path so I decided to go the simple way.

Copy link
Member

Choose a reason for hiding this comment

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

What about

ol.control.Control.prototype.setTarget = function(target) {
  var map = this.map_;
  this.setMap(null);
  this.target_ = target;
  this.setMap(map);
};

If that does not work, you could at least add an assertion to ensure that the map is not set yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

That may work but it's also three setMap calls if you add the control to the map and then calls setTarget. This will also add the control element to the map overlay container to finally remove it to add it to the final target. Not so nice I think.

Copy link
Member

Choose a reason for hiding this comment

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

It may also be important to maintain the control's position in the collection (meaning it might be unexpected to have it jump to the end of the collection when you call setTarget).

I imagine most people will expect setTarget to work when called more than once. But I wouldn't be opposed to adding the method as is and then making it more useful later.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Looks like a major effort indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It may also be important to maintain the control's position in the collection (meaning it might be unexpected to have it jump to the end of the collection when you call setTarget).

setMap is called when a control is added to or removed from a map's controls collection, but calling setMap directly does not change the collection. So that part is not an issue.

This commit adds a setTarget method to ol.control.Control. This function can be used in cases where the target cannot be set at control construction time. For example, with Angular, it makes sense to create the control instance in an Angular controller, and have a "control" directive insert the control into the DOM.
@elemoine
Copy link
Member Author

Thanks for the reviews. I changed the docs as suggested by @tschaub. I'll merge this if there are no other comments.

@elemoine
Copy link
Member Author

@ahocevar do you agree with merging this?

@ahocevar
Copy link
Member

Sure, please merge!

@elemoine
Copy link
Member Author

Thanks. I just wanted to make sure.

elemoine pushed a commit that referenced this pull request Jan 16, 2015
Add ol.control.Control#setTarget
@elemoine elemoine merged commit 4b3726f into openlayers:master Jan 16, 2015
@elemoine elemoine deleted the control-setmap branch January 16, 2015 10:16
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.

3 participants