Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#6219 closed enhancement (fixed)

BorderContainer: Add public drag notifications

Reported by: ptwobrussell Owned by: Sam Foster
Priority: high Milestone: 1.2
Component: Dijit Version: 1.0
Keywords: Cc: Sam Foster, ptwobrussell, Adam Peller
Blocked By: Blocking:

Description (last modified by Sam Foster)

Following discussion in #dojo, create a getSplitter api on the BC to allow connection to the invidiual splitter events, and a test case showing it in action

Attachments (3)

6219_splitterEvents.patch (2.8 KB) - added by Sam Foster 11 years ago.
[PATCH][CLA] Naive implementation of the splitter events for further discusison
6219_getSplittersAndEvents.patch (5.7 KB) - added by Sam Foster 11 years ago.
[PATCH] [CLA] adds getSplitter, getSplitters to BC, onMove, onMoveStart, onMoveEnd splitter methods. Plus simple test/use case
6219_BCgetSplitter.patch (4.5 KB) - added by Sam Foster 11 years ago.
[PATCH] [CLA] adds getSplitter to BC, adds a test case

Download all attachments as: .zip

Change History (18)

comment:1 Changed 12 years ago by bill

Cc: Sam Foster added
Milestone: 1.2
Summary: Add public drag notifications to BorderContainerBorderContainer: Add public drag notifications

Please list (some of) the use cases.

comment:2 Changed 12 years ago by Adam Peller

and why standard DOM event notifications aren't sufficient

comment:3 Changed 11 years ago by dylan

Owner: set to Sam Foster

comment:4 Changed 11 years ago by Adam Peller

oh, are we talking about the splitters?

comment:5 Changed 11 years ago by Sam Foster

Cc: ptwobrussell Adam Peller added

I was going to approach this by adding a topic name to the properties of the splitter that we can publish events to. This is of use when you might want content to go into a different state while the splitter is being moved, persist the BC state, defer other updates to the page and so on. There might also be times when you want to abort the drag - though I'm not sure this ticket covers all of that.

If anyone has suggestions for the new property name - it needs to be on the BC prototype, I think and passed onto the splitters' ctor. And, any expectations for what the event payload would be. I'm thinking the event name (dragstart/end/move) container's region, and splitter position

Changed 11 years ago by Sam Foster

Attachment: 6219_splitterEvents.patch added

[PATCH][CLA] Naive implementation of the splitter events for further discusison

comment:6 Changed 11 years ago by Sam Foster

Description: modified (diff)

Its important that if we do allow a user to subscribe to splitter events including ther ondrag/onmove event - which fires very frequently - that we dont incurr a performance loss disproportionate to the amount of work that handler really needs to do. With that in mind, it would be ideal to be pretty granular about exactly which splitter, and which phase of the drag you were interested in recieving events from. That presents a potential problem in the proliferation of topics - particulaly if you wanted to e.g. subscribe to any splitter drag event

comment:7 Changed 11 years ago by bill

Hmm, I'd need to be convinced that topics are better than just connect points on the widget.

The other question is how this is an improvement over monitoring onResize on the individual panes?

Changed 11 years ago by Sam Foster

[PATCH] [CLA] adds getSplitter, getSplitters to BC, onMove, onMoveStart, onMoveEnd splitter methods. Plus simple test/use case

comment:8 Changed 11 years ago by Sam Foster

6219_getSplittersAndEvents.patch implements I think what we discussed. I made getSplitter(/*String*/region) and getSplitters. on the splitter class, onMoveStart, onMove and onMoveEnd stubs are called for the respective drag phases.

I'm open to ideas on what the event payload should be. Currently its type,target (the splitter widget), x, y. The coords are the top/left style values (ie. relative to the BC not the window). I could mixin to the native event if that would be more useful - I'd have to add a prefix or something to the artificial event property names at that point.

Changed 11 years ago by Sam Foster

Attachment: 6219_BCgetSplitter.patch added

[PATCH] [CLA] adds getSplitter to BC, adds a test case

comment:9 Changed 11 years ago by Sam Foster

Description: modified (diff)

6219_BCgetSplitter.patch reworked to just add a getSplitter method to BorderContainer?, a couple caching/optimizing vars in Splitter. Also fixes an IE bug with BC.getChildren which was wanting to call filter on an array. The watchedBC test case shows connects to the _startDrag, _endDrag and domNode.mousemove to update the UI - which is the actual goal of this ticket.

comment:10 Changed 11 years ago by bill

Yah, looks reasonable to me; hoping Adam will check it.

The thing about

var
       a,
       b,
       c

(without repeating "var"). I like it but Adam tells me that it confuses shrinksafe. Or at least prevents it from doing optimizations of shortening the variable names.

comment:11 Changed 11 years ago by Sam Foster

With Adam's blessing I'll roll back the var optimization and commit this.

The filter thing btw highlights a testing hole - that broken getChildren (ie IE) method had gone undetected apparently

comment:12 Changed 11 years ago by bill

OK thanks. About the filter() thing, I thought that Container.getChildren() was returning NodeList, as per the code:

dojo.query("> [widgetId]", this.containerNode).map(dijit.byNode)

(I thought that map() returned something NodeList-esque even if it didn't contain nodes. Hmm, I guess not. Thanks for catching it.

comment:13 Changed 11 years ago by Sam Foster

Resolution: fixed
Status: newclosed

(In [14839]) fixes #6219

  • adds getSplitter method to BorderContainer?
  • fixes a bug with getChildren in IE (getChildren returns an array not a NodeList?)
  • adds splitter-watching test case

!strict

comment:14 Changed 11 years ago by Adam Peller

Sorry for the late review, but yes, what Bill said... each var needs its own 'var' stmt for now, unfortunately. See #1768.

I wasn't sure it was worth a local var for two uses of this.horizontal -- the actual savings is probably minimal, but I guess it couldn't hurt.

Also, we need API docs for getSplitter, as simple as it might be.

comment:15 Changed 11 years ago by Adam Peller

(In [15026]) doc public method, style changes. Refs #6219 !strict

Note: See TracTickets for help on using tickets.