Opened 8 years ago
Closed 8 years ago
#14946 closed defect (fixed)
placeAt() API confusing
Reported by: | bill | Owned by: | bill |
---|---|---|---|
Priority: | undecided | Milestone: | 1.8 |
Component: | Dijit | Version: | 1.7.2 |
Keywords: | Cc: | dante | |
Blocked By: | Blocking: |
Description (last modified by )
See attached file, It doesn't layout correctly, although if startup() is called at the very end it does.
Attachments (3)
Change History (8)
Changed 8 years ago by
Attachment: | earlyStartup.html added |
---|
comment:1 Changed 8 years ago by
Cc: | dante added |
---|---|
Description: | modified (diff) |
Summary: | startup() called early doesn't layout correctly → placeAt() API confusing |
Turns out it's a problem with the test case, or arguably a design problem in the spec of the placeAt() function. In other words, placeAt() is working according to spec, but the spec turns out to be confusing. I'm CC'ing Pete to see if he has anything to say, since placeAt() is his function.
The current rules are:
(1) if the reference widget has an addChild() method then pass it in without quotes. So, this works:
myWidget.placeAt(myLayoutWidget)
But this fails:
myWidget.placeAt("myLayoutWidget")
If placeAt() is passed a string, it's treated as a reference to a DOMNode, not as a reference to a widget.
(2) If the reference widget does not have an addChild() method, the opposite is true. This is correct:
myWidget.placeAt("myContentPane")
or slightly clearer:
myWidget.placeAt(myContentPane.domNode)
but this will fail:
myWidget.placeAt(myContentPane)
Of course, one way to avoid this problem is to stop using placeAt(). The test case could be rewritten to use addChild() for the layout widgets and set("content", ...) for the ContentPane.
Changed 8 years ago by
Attachment: | earlyStartupFixed.html added |
---|
test case with placeAt() calls corrected to be used according to it's spec... it's working
comment:2 Changed 8 years ago by
Turns out this is complicated to implement to work intuitively, since:
childWidget.placeAt(parentWidget, 5);
should call parentWidget.addChild(), whereas
childWidget.placeAt(parentWidget, "before"); childWidget.placeAt(parentWidget, "after");
should be calling domConstruct.place().
Further, we need special handling for first and last when an addChild() method is available:
childWidget.placeAt(parentWidget, "first"); --> parentWidget.addChild(childWidget, 0); childWidget.placeAt(parentWidget, "last"); --> parentWidget.addChild(childWidget, parentWidget.getChildren().length);
(This applies equally whether parentWidget is a widget reference, or a widget id.)
comment:3 Changed 8 years ago by
Even worse are the "before", "after", "replace" and "only" options, which currently somewhat work when you pass a DOMNode or string to placeAt(). They don't generally do what's expected when you pass an id of a widget, but they do something.
A correct implementation of childWidget.placeAt(reference, "before")
, childWidget.placeAt(reference, "after")
and childWidget.placeAt(reference, "replace")
need to call addChild() on the grandparent widget, plus for position="replace", removeChild() and destroyRecursive(), and would be something like:
var refWidget = registry.byId(reference); gpw = refWidget && refWidget.getParent(); if(gpw && gpw.addChild){ var parentIndex = gpw.getIndexOfChild(refWidget); gpw.addChild(this, position == "before" ? parentIndex : parentIndex+1); }else{ domConstruct.place(this.domNode, reference, position); } if(position == "replace" && refWidget){ gpw.removeChild && gpw.removeChild(refWidget); refWidget.destroyRecursive(); }
and for childWidget.placeAt(reference, "only")
:
if(refWidget){ array.forEach(refWidget.getChildren(), function(child){ refWidget.removeChild && refWidget.removeChild(child); child.destroyRecursive(); } } domConstruct.empty(refWidget ? refWidget.containerNode || refWidget.domNode : reference); // in case any stray DOM left this.placeAt(reference, 0);
Changed 8 years ago by
Attachment: | placeAtFullyFunctional.js added |
---|
Code for a placeAt() that supports all the positional parameters that domConstruct.place() does. But I don't want to check this in since it's a fair amount of code.
comment:4 Changed 8 years ago by
Milestone: | tbd → 1.8 |
---|---|
Owner: | set to bill |
Status: | new → assigned |
teslan's test case