Aeva / m.grl

Midnight Graphics & Recreation Library
http://mgrl.midnightsisters.org
GNU Lesser General Public License v3.0
44 stars 3 forks source link

Allow bind_to_node as part of please.overlay.new_element() #262

Closed wijnen closed 7 years ago

wijnen commented 7 years ago

Almost every overlay element should be bound to a node. Even for non-moving elements, it makes it possible to position them in space that way. Because of this, please.overlay.new_element() is almost always followed by a call to bind_to_node() on the newly created object. This makes the code longer and harder to read IMO, so I would instead create a wrapper function that makes both calls. However, as this doesn't seem to be a personal issue for me, but something that more people would likely appreciate, it's probably a good idea to include this feature in M.GRL itself. So my request is: please change new_element to accept the node as an argument. I would make it the first argument, because I normally use neither the id nor the classes arguments, but of course that would break the interface. If you prefer to add it as a third argument, that's still better than not having it, I think.

If you agree this is useful, I'm happy to write a pull request for it.

Aeva commented 7 years ago

I think this is sensible. I think the implementation should just be to change please.new_element so that if there is only one argument provided, to check to see if it is a node. That way, please.new_element(id, classes), please.new_element(id), and please.new_element(some_node) would all be valid.

I would gladly accept such a PR! Branching from either master or fast_graph is fine, I think?