Opened 11 years ago
Closed 11 years ago
#12268 closed defect (fixed)
[patch][ccla] Avoid _Templated dependency on charting legend
Reported by: | cjolif | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | high | Milestone: | 1.7 |
Component: | Charting | Version: | 1.6.0b1 |
Keywords: | 1.7-mobile | Cc: | |
Blocked By: | Blocking: |
Description
charting Legend is extending _Templated. However it can be very easily re-written without that dependency which will save download time in a mobile context and will fit with dojox.mobile good practice of not including _Templated for mobile deployment.
This attached patch is replacing the template by a buildRendering implementation.
Attachments (2)
Change History (15)
Changed 11 years ago by
comment:1 Changed 11 years ago by
Please attach patch files, not full files. Also, the buildRendering() you have could be more succinctly written with something like:
buildRendering: function(){ this.domNode = this.legendNode = dojo.create("table", {role: "group", "aria-label": "chart legend", "class": "dojoxLegendNode"}); this.legendBody = dojo.create("tbody", this.legendNode); },
BTW, the legendNode attribute in this class is unnecessary since it's the same as this.domNode.
comment:2 follow-up: 3 Changed 11 years ago by
Milestone: | tbd → future |
---|---|
Status: | new → assigned |
Type: | enhancement → defect |
Please attach a patch taking Bill's comments into consideration.
I'll treat the unnecessary dependence on _Templated
as a defect.
comment:3 Changed 11 years ago by
Sorry I inadvertently attached the full file instead of the patch file. I'll take Bill's comment into consideration and attached a new patch. Please note that removing the unnecessary legendNode attribute also forces to patch SelectableLegend?.js as it was using this property.
I kept the this.inherited(arguments) call removed by Bill because parent method does something with CSS baseClass and I guess this can be useful in some cases.
Let me know if further modification are needed.
comment:4 Changed 11 years ago by
Keywords: | 1.7-mobile added |
---|
comment:7 Changed 11 years ago by
I'm still getting 1 reject applying this patch to trunk:
applying patch 12268-nontemplatedlegend...
(Stripping trailing CRs from patch.) patching file dojox/charting/widget/Legend.js Hunk #1 FAILED at 1. 1 out of 3 hunks FAILED -- saving rejects to file dojox/charting/widget/Legend.js.rej (Stripping trailing CRs from patch.) patching file dojox/charting/widget/SelectableLegend.js
comment:8 Changed 11 years ago by
Milestone: | future → 1.7 |
---|
comment:10 Changed 11 years ago by
Chris, the latest patch indeed applies on trunk but I think I found an issue with it at runtime. When porting from old trunk to new trunk I forgot to re-apply a fix I included compare to bill's in the first patch. That is:
this.legendBody = dojo.create("tbody", null, this.legendNode);
not
this.legendBody = dojo.create("tbody", this.legendNode);
I'll re-attached a fixed patch in a second.
Changed 11 years ago by
Attachment: | nontemplatedlegend.patch added |
---|
comment:11 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:12 Changed 11 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
reopening...had out of date patch file
patch