Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#16145 closed enhancement (fixed)

DataList/StoreList should allow custom ListItem

Reported by: ykami Owned by: Eric Durocher
Priority: undecided Milestone: 1.9
Component: DojoX Mobile Version: 1.8.0
Keywords: Cc: bill
Blocked By: Blocking:

Description

Users may want to subclass the ListItem widget for their own purposes, but there is no way to use a custom ListItem with DataList/StoreList since dojox.mobile.ListItem is hard-coded in them.

Attachments (2)

16145.patch (1.6 KB) - added by ykami 7 years ago.
16145-alternate.patch (1.7 KB) - added by cjolif 6 years ago.
alternate patch that achieves similar results

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by ykami

Attachment: 16145.patch added

comment:1 Changed 6 years ago by cjolif

Milestone: tbd1.9

This is definitely needed, I think it should go in 1.9.

I would prefer another naming however. Starting with underscore would mean private and here the user obviously has to override/replace that method (should be marked "protected" or "extension" so that he understands this must not be called directly).

comment:2 Changed 6 years ago by ykami

I took the idea, naming convention, and inline doc from dijit/Tree._createTreeNode(). (I avoided _createListItem because createListItem was already used.)

comment:3 Changed 6 years ago by cjolif

Cc: bill added

I'm all for copying Dijit practices in general. This is really helping users using both dojox/mobile & dijit without learning things twice. Here it sounds to me that maybe we can make an exception. Bill do you have an opinion, is that really a good idea to have such method starting with underscore when users needs to override it? For example dijit/tree/model is different. The "newItem" method is not underscored. I think in general underscore must be kept from private. Users should not have to look at underscore things otherwise there is no clear distinction if we start blurring that rule.

By the way ykami, is the already existing createListItem supposed to be called by users?

comment:4 Changed 6 years ago by ykami

By the way ykami, is the already existing createListItem supposed to be called by users?

Supposed to be called or overridden by subclasses.

comment:5 Changed 6 years ago by bill

Bill do you have an opinion, is that really a good idea to have such method starting with underscore when users needs to override it?

Well, a user may need to override any method, so by that way of thinking there should be no underscores anywhere.

My original thinking was to use underscore for any private or protected method, to hide all advanced methods from novice users. I figured that advanced users can tell the difference between private and protected methods, but novice users will get easily confused by seeing too many methods.

But that policy doesn't match the most popular protected methods of all, like postCreate(). So, things are a mess right now.

I don't have a strong opinion either way about how things should be named.

comment:6 Changed 6 years ago by cjolif

I guess this is not the place to discuss this into more details. But if obviously a user might want to override any method I personally make a clear difference between the methods that we explicitly design to be overridden from the ones that are not explicitly designed for that. Here this method is explicitly introduced for the user to override it so starting by hiding it behind an underscore is strange to me. It has no other objective than do. In particular if you look at the API doc, it won't appear by default which I think is problematic in this case.

comment:7 Changed 6 years ago by ykami

I don't have a strong opinion about naming, either. I just think consistency is more important than how things are named.

Changed 6 years ago by cjolif

Attachment: 16145-alternate.patch added

alternate patch that achieves similar results

comment:8 Changed 6 years ago by cjolif

Just uploaded an alternate patch just exposing a property to allow the user to specify which ListItem class to use. Advantages that I see:

1/ it is a little bit more compact (no new method)

2/ it can be used in markup (setting that property value in the markup)

If there is no objection I think I'll commit that one.

comment:9 Changed 6 years ago by ykami

Is that approach used somewhere in dijit?
Looks like the property takes a module object. It is not clear to me how to use it in markup.
I would agree if it took an mid as a string just like the screenSizeAwareClass property in FixedSplitter.

comment:10 Changed 6 years ago by cjolif

From discussions with Bill I think that's where Bill wants to go with Dijit for that kind of properties even though this is not yet here to preserve compatibility (for example datePackage property). But here the property does not exist yet so better go with how Dijit will do in the future that being stuck on old practices.

Also for now the (non mobile) parser use would be the following:

<script type="dojo/require">
  MyListItem: "my/package/MyListItem">
</script>
<div data-dojo-props="itemRenderer: MyListItem"></div>

longer term I hope we can hint the parser for this so that it can recognize the MID directly like it does for data-dojo-type & data-dojo-mixins.

(this is true this is not working with mobile parser, but the method overriding solution is not either anyway...)

As discussed with Bill taking the MID string directly in the class is problematic because it forces each class to deal with the asynchronicity. Also I must admit I don't like to rely on string when I'm doing programmatic code. We do have a reference to the class let's use it instead of relying on string we would have to separately change if we for example rename a class.

Last edited 6 years ago by cjolif (previous) (diff)

comment:11 in reply to:  10 Changed 6 years ago by ykami

OK, thanks for the clarification. I have no objection if that is the way dijit goes.

(this is true this is not working with mobile parser, but the method overriding solution is not either anyway...)

If this is true, it doesn't sound very useful for declarative solutions... But I believe it won't be a problem at least for programmatic solutions.

ps.
One of the purposes of using a string MID is lazy-loading - that is, to be able to load the class only when it becomes necessary in the parent widget logic. In this ListItem case, however, lazy-loading is not necessary. So it doesn't have to be a string.

comment:12 Changed 6 years ago by cjolif

In [30229]:

refs #16145. Allow one to easily switch the ListItem? class used when building data-aware lists (+deprecation of data lists in favor of store in prep of 2.0).

comment:13 Changed 6 years ago by cjolif

Resolution: fixed
Status: newclosed

comment:14 Changed 6 years ago by cjolif

In [30280]:

refs #16145. Commit missing test file.

Note: See TracTickets for help on using tickets.