Opened 9 years ago

Closed 9 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)

Legend.js (5.2 KB) - added by cjolif 9 years ago.
patch
nontemplatedlegend.patch (2.3 KB) - added by cjolif 9 years ago.

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by cjolif

Attachment: Legend.js added

patch

comment:1 Changed 9 years ago by bill

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 Changed 9 years ago by Eugene Lazutkin

Milestone: tbdfuture
Status: newassigned
Type: enhancementdefect

Please attach a patch taking Bill's comments into consideration.

I'll treat the unnecessary dependence on _Templated as a defect.

comment:3 in reply to:  2 Changed 9 years ago by cjolif

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 9 years ago by Chris Mitchell

Keywords: 1.7-mobile added

comment:5 Changed 9 years ago by Chris Mitchell

patch appears to be out of date with trunk. please update

comment:6 Changed 9 years ago by cjolif

The new attachment should work on trunk.

comment:7 Changed 9 years ago by Chris Mitchell

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 9 years ago by Chris Mitchell

Milestone: future1.7

comment:9 Changed 9 years ago by Chris Mitchell

Disregard, latest patch working on trunk. user error.

comment:10 Changed 9 years ago by cjolif

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 9 years ago by cjolif

Attachment: nontemplatedlegend.patch added

comment:11 Changed 9 years ago by Chris Mitchell

Resolution: fixed
Status: assignedclosed

(In [24511]) fixes #12268 Avoid _Templated dependency on charting legend. !strict

comment:12 Changed 9 years ago by Chris Mitchell

Resolution: fixed
Status: closedreopened

reopening...had out of date patch file

comment:13 Changed 9 years ago by Chris Mitchell

Resolution: fixed
Status: reopenedclosed

fixed in [24537]

Note: See TracTickets for help on using tickets.