Merge pull request #2 from ian-pvd/wordpress-vip-code-review-patch-tests

Update documentation for generateLegend method; fix corresponding unit tests
ian-pvd · Apr 1, 2019 · 62c4512 · 62c4512
2 parents 2e8276f + 610d191
commit 62c4512
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 16 deletions.
diff --git a/docs/configuration/legend.md b/docs/configuration/legend.md
@@ -161,7 +161,7 @@ Now when you click the legend in this chart, the visibility of the first two dat
 
 ## HTML Legends
 
-Sometimes you need a very complex legend. In these cases, it makes sense to generate an HTML legend. Charts provide a `generateLegend()` method on their prototype that returns an HTML string for the legend.
+Sometimes you need a very complex legend. In these cases, it makes sense to generate an HTML legend. Charts provide a `generateLegend()` method on their prototype that returns an HTML fragment for the legend.
 
 To configure how this legend is generated, you can change the `legendCallback` config property.
 
@@ -171,7 +171,7 @@ var chart = new Chart(ctx, {
     data: data,
     options: {
         legendCallback: function(chart) {
-            // Return the HTML string here.
+            // Return the HTML fragment here.
         }
     }
 });

diff --git a/src/controllers/controller.doughnut.js b/src/controllers/controller.doughnut.js
@@ -19,6 +19,7 @@ defaults._set('doughnut', {
 		mode: 'single'
 	},
 	legendCallback: function(chart) {
+		var fragment = document.createDocumentFragment();
 		var list = document.createElement('ul');
 		list.setAttribute('class', chart.id + '-legend');
 
@@ -35,9 +36,11 @@ defaults._set('doughnut', {
 					listItem.appendChild(document.createTextNode(labels[i]));
 				}
 			}
+
+			fragment.appendChild(list);
 		}
 
-		return list;
+		return fragment;
 	},
 	legend: {
 		labels: {

diff --git a/src/controllers/controller.polarArea.js b/src/controllers/controller.polarArea.js
@@ -31,7 +31,8 @@ defaults._set('polarArea', {
 	},
 
 	startAngle: -0.5 * Math.PI,
-	legendCallback: function(chart) {
+	legendCallback: function (chart) {
+		var fragment = document.createDocumentFragment();
 		var list = document.createElement('ul');
 		list.setAttribute('class', chart.id + '-legend');
 
@@ -48,9 +49,11 @@ defaults._set('polarArea', {
 					listItem.appendChild(document.createTextNode(labels[i]));
 				}
 			}
+
+			fragment.appendChild(list);
 		}
 
-		return list;
+		return fragment;
 	},
 	legend: {
 		labels: {

diff --git a/src/plugins/plugin.legend.js b/src/plugins/plugin.legend.js
@@ -71,17 +71,24 @@ defaults._set('global', {
 	},
 
 	legendCallback: function(chart) {
+		var fragment = document.createDocumentFragment();
 		var list = document.createElement('ul');
 		list.setAttribute('class', chart.id + '-legend');
-		for (var i = 0; i < chart.data.datasets.length; i++) {
-			var listItem = list.appendChild(document.createElement('li'));
-			var listItemSpan = listItem.appendChild(document.createElement('span'));
-			listItemSpan.style.backgroundColor = chart.data.datasets[i].backgroundColor;
-			if (chart.data.datasets[i].label) {
-				listItem.appendChild(document.createTextNode(chart.data.datasets[i].label));
+
+		if (datasets.length) {
+			for (var i = 0; i < chart.data.datasets.length; i++) {
+				var listItem = list.appendChild(document.createElement('li'));
+				var listItemSpan = listItem.appendChild(document.createElement('span'));
+				listItemSpan.style.backgroundColor = chart.data.datasets[i].backgroundColor;
+				if (chart.data.datasets[i].label) {
+					listItem.appendChild(document.createTextNode(chart.data.datasets[i].label));
+				}
 			}
+
+			fragment.appendChild(list);
 		}
-		return list;
+
+		return fragment;
 	}
 });
 

diff --git a/test/specs/global.defaults.tests.js b/test/specs/global.defaults.tests.js
@@ -102,8 +102,12 @@ describe('Default Configs', function() {
 				options: config
 			});
 
-			var expectedLegend = '<ul class="' + chart.id + '-legend"><li><span style="background-color:red"></span>label1</li><li><span style="background-color:green"></span>label2</li></ul>';
-			expect(chart.generateLegend()).toBe(expectedLegend);
+			var expectedLegend = document.createDocumentFragment();
+			var expectedLegendList = document.createElement('ul');
+			expectedLegendList.setAttribute('class', chart.id + '-legend');
+			expectedLegendList.innerHTML = '<li><span style="background-color: red;"></span>label1</li><li><span style="background-color: green;"></span>label2</li>';
+			expectedLegend.appendChild(expectedLegendList);
+			expect(chart.generateLegend().firstElementChild.outerHTML).toBe(expectedLegend.firstElementChild.outerHTML);
 		});
 
 		it('should return correct legend label objects', function() {
@@ -218,8 +222,12 @@ describe('Default Configs', function() {
 				options: config
 			});
 
-			var expectedLegend = '<ul class="' + chart.id + '-legend"><li><span style="background-color:red"></span>label1</li><li><span style="background-color:green"></span>label2</li></ul>';
-			expect(chart.generateLegend()).toBe(expectedLegend);
+			var expectedLegend = document.createDocumentFragment();
+			var expectedLegendList = document.createElement('ul');
+			expectedLegendList.setAttribute('class', chart.id + '-legend');
+			expectedLegendList.innerHTML = '<li><span style="background-color: red;"></span>label1</li><li><span style="background-color: green;"></span>label2</li>';
+			expectedLegend.appendChild(expectedLegendList);
+			expect(chart.generateLegend().firstElementChild.outerHTML).toBe(expectedLegend.firstElementChild.outerHTML);
 		});
 
 		it('should return correct legend label objects', function() {