Opened 8 years ago
Closed 4 years ago
#15045 closed enhancement (patchwelcome)
dijit.form._FormSelectWidget.js __fillContent attribute handling
Reported by: | Jonathan | Owned by: | |
---|---|---|---|
Priority: | undecided | Milestone: | 1.13 |
Component: | Dijit - Form | Version: | 1.7.2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description (last modified by )
Since at least Dojo 1.5, the lines 490+ in dijit.form._FormSelectWidget::_fillContent()
say:
// FIXME: disabled and selected are not valid on complex markup children (which is why we're // looking for data-dojo-value above. perhaps we should data-dojo-props="" this whole thing?) // decide before 1.6
Moving to data-dojo-props would not only solve the "disabled" and "selected" issues but would also developers to pass data from markup to JS without having to override _fillContent().
Change History (12)
comment:1 Changed 8 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 8 years ago by
Good point.
The reason I found this comment in the code was having to pass some of my custom "data-" attributes to my widget. I ended up overriding the entire _fillContent() method which defeats encapsulation and DRY principals, but had no other choice (that I could see).
What would you suggest?
comment:3 Changed 8 years ago by
I would leave the attributes on the <option> tags, and since the code right above looks for "data-dojo-value" (in addition to "value"), I think it should also look for "data-dojo-disabled" and "data-dojo-selected" (in addition to "disabled" and "selected").
comment:4 Changed 8 years ago by
Unless I'm misunderstanding or missing something, when you "leave" the attributes on the <option> tag, they get removed when the widget replaces the <select> with a <table>. That's why I had to overwrite dijit.form._FormSelectWidget::_fillContent()
.
Couldn't that method ~pass/parse~ _all_ "data-" attributes into the dijit.form.__SelectOption
object? Or is that what you mean by "leave the attributes"?
comment:5 Changed 8 years ago by
Right, I meant to make _fillContent() parse children of this.srcNodeRef like <div data-dojo-disabled=true ...>
in addition to <option disabled>
. Shouldn't have used the word "leave".
comment:6 Changed 8 years ago by
Sounds good. Just to confirm though... :)
Would custom attributes, e.g. <option data-my-custom-attribute ...>
also be parsed?
comment:7 Changed 8 years ago by
No, not with the current (pattern of) code, it just looks for the list of attributes specified in the code.
comment:8 follow-up: 9 Changed 8 years ago by
Is there a good reason not to change that?
At the very least, could the code that ~builds~ the dijit.form.__SelectOption
not be refactored/extracted into its own method (eg. _mapOption()
)?
_fillContent: function() { ... // We've extracted the method applied in map() to its own method _mapOption(). // This makes it easier to override cleanly. opts = this.options = this.srcNodeRef ? dojo.query(">", this.srcNodeRef).map(this._mapOption, this) : []; ... }, _mapOption: function(node){ if(node.getAttribute("type") === "separator"){ return { value: "", label: "", selected: false, disabled: false }; } return { value: (node.getAttribute("data-" + kernel._scopeName + "-value") || node.getAttribute("value")), label: String(node.innerHTML), selected: node.getAttribute("selected") || false, disabled: node.getAttribute("disabled") || false }; }
Doing so would allow developers to have a very clean override that calls this.inherited
and ~extends~ the dijit.form._SelectOption
with the attributes they need.
Would a patch be useful? If so, what are the steps for submitting a patch to Dojo?
comment:9 Changed 8 years ago by
Replying to jonathan.dh:
Is there a good reason not to change that?
ISTM it would be adding a bunch of parsing code for a corner case.
At the very least, could the code that ~builds~ the
dijit.form.__SelectOption
not be refactored/extracted into its own method (eg._mapOption()
)?
That seems reasonable to me.
Would a patch be useful? If so, what are the steps for submitting a patch to Dojo?
If you've filed a CLA you can just attach a patch file to this ticket.
comment:10 Changed 6 years ago by
Owner: | Douglas Hays deleted |
---|---|
Status: | new → assigned |
comment:11 Changed 6 years ago by
Status: | assigned → open |
---|
comment:12 Changed 4 years ago by
Milestone: | tbd → 1.12 |
---|---|
Resolution: | → patchwelcome |
Status: | open → closed |
Given that no one has shown interest in creating a patch in the past 3+ years, I'm closing this as patchwelcome.
It sounds like a lot of data to squeeze into data-dojo-props though... what if it was a list of 50 values?