joseph / Monocle

A silky, tactile browser-based ebook JavaScript library.
http://monocle.inventivelabs.com.au
MIT License
743 stars 200 forks source link

Enable controls that live outside of `monelem_box` #173

Closed julien-c closed 11 years ago

julien-c commented 11 years ago

I'm implementing a scrubber that lives on my main page, outside of Monocle's containers.

I figured out I could pass a DOM node to createControlElements, however, I must first "extend" my node with Monocle.Factory() so that controls can call the node.dom suite of DOM helpers.

Here's how I ended up doing it:

var scrubber = document.getElementById('scrubber');
scrubber.dom = new Monocle.Factory(scrubber, 'scrubber', null, reader);
Monocle.scrubber = new Monocle.Controls.Scrubber(reader);
reader.addControl(Monocle.scrubber, 'explicit', {node: scrubber});

Feel free to suggest anything simpler, or more elegant!

Best,

Julien

svrx commented 11 years ago

I'm new to this codebase but have been working lately a bit on it, and to me it seem like a good solution. The only aspect that comes to my mind if you realy need to apply the Monocle.Factory() before calling the reader.addControl(...), or it is workable do it after if needed.

This way the API would be simpler.

joseph commented 11 years ago

Oh yeah, what a hassle. This is a good proposal, but as you say, it's still a bit of work to get the control registered on your script's side of things. It'd be great if we could make the API cleaner for this scenario. I'm going to put together a simple test case so we've got something to hack on.

joseph commented 11 years ago

I hope you don't mind me putting forward a slightly different implementation (for the second time today). I wonder if the change in f63bc5c and ab0a4f9 gives you what you want with a cleaner API? The main difference (suggested by @svrx) is that you don't have to futz around with a new Factory object. Another key difference is that we don't need a different control type -- you can just use 'standard'. And finally, if you want to pass an id as the container argument rather than an element, Monocle will do the getElementById lookup for you. So your example above would become:

Monocle.scrubber = new Monocle.Controls.Scrubber(reader);
reader.addControl(Monocle.scrubber, 'standard', { container: 'scrubber' });
joseph commented 11 years ago

You have a couple of other commits in this PR that provide more information to selection events. In general I like it, but it would be an API-breaking change. I think there's another way to do what you're doing without changing the API: evt.m.rangeStartContainer.ownerDocument. Does this suit you?

— J

svrx commented 11 years ago

I've checked your implementation and to me seems like a better solution... :)

julien-c commented 11 years ago

Just got to test this, but it seems to work perfectly! Thanks @joseph, would love to see this merged into master. Also, you're totally right about selection events info.

joseph commented 11 years ago

Closed as the other branch has been merged into master. Thanks for your contribution!