Skip to content
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

Support .placeAt(node, pos) #1135

Closed
gavinr opened this issue Jun 3, 2015 · 5 comments
Closed

Support .placeAt(node, pos) #1135

gavinr opened this issue Jun 3, 2015 · 5 comments

Comments

@gavinr
Copy link

gavinr commented Jun 3, 2015

Similar to other dijits, allow me to place my dgrid in the form:

this.myGrid = new CustomGrid({ ... });
this.myGrid.placeAt(myNode, 'last');
@kfranqueiro
Copy link
Member

I just pushed a commit to a branch that adds this, if you want to take a look at the diff and apply it yourself for now. I want to have unit tests for it before merging it.

@dylans
Copy link
Member

dylans commented Jun 5, 2015

I think one of the reasons this wasn't included before was the perception that _WidgetBase was pretty large in size, though perhaps that has changed now? It might be worth making it a separate extension if it is fairly large, though most apps that use dgrid probably include _WidgetBase anyway. It's probably worth comparing build sizes?

@kfranqueiro
Copy link
Member

Funny you mention that. I had some thoughts hit me in the last hour that might explain why this wasn't added earlier.

  • It's quite possible that at the time we first thought about this, the DijitRegistry extension didn't exist. Notice that's where I added this - it's pretty safe to assume that if you're using that extension, you're already loading _WidgetBase anyway.
  • On top of not wanting this in dgrid/List directly due to _WidgetBase, there is (or was, up through/including 0.4.x) the fact that dgrid didn't use dojo/dom-construct, it used put-selector/put. Even implementing just the DOM parts of placeAt directly would've involved bringing in a module that was otherwise redundant at the time.

We could potentially consider putting the DOM-specific part of this directly in dgrid/List in master (which no longer relies on put-selector), and the widget portion of it (perhaps still leaning on _WidgetBase directly in non-DOM cases) in the DijitRegistry extension. But I wouldn't want to do that in 0.4, and it might be confusing (1) separating it to 2 places and (2) changing it between 0.4 and master, so I'm inclined to keep it simple the way I currently added it above, which only exposes it through the DijitRegistry extension.

kfranqueiro added a commit that referenced this issue Sep 25, 2015
Includes unit tests.

Resolves #1135.

(cherry picked from commit 1767584)
@kfranqueiro
Copy link
Member

Since this didn't get any further comments, I've gone ahead and added tests to my previous commit and merged it. This will be in 0.5.0 and 0.4.1.

@gavinr
Copy link
Author

gavinr commented Sep 26, 2015

Thanks @kfranqueiro !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants