[GitHub] williaster commented on a change in pull request #5670: Refactor treemap

[GitBox <gi...@apache.org>]

williaster commented on a change in pull request #5670: Refactor treemap
URL: https://github.com/apache/incubator-superset/pull/5670#discussion_r212477774
 
 

 ##########
 File path: superset/assets/src/visualizations/treemap.js
 ##########
 @@ -1,61 +1,107 @@
-/* eslint-disable no-shadow, no-param-reassign, no-underscore-dangle, no-use-before-define */
+/* eslint-disable no-shadow, no-param-reassign */
 import d3 from 'd3';
+import PropTypes from 'prop-types';
 import { getColorFromScheme } from '../modules/colors';
-
-require('./treemap.css');
+import './treemap.css';
+
+// Declare PropTypes for recursive data structures
+// https://github.com/facebook/react/issues/5676
+const lazyFunction = f => (() => f().apply(this, arguments));
+
+const leafType = PropTypes.shape({
+  name: PropTypes.string,
+  value: PropTypes.number.isRequired,
+});
+
+const parentShape = {
+  name: PropTypes.string,
+  children: PropTypes.arrayOf(PropTypes.oneOfType([
+    PropTypes.shape(lazyFunction(() => parentShape)),
+    leafType,
+  ])),
+};
+
+const nodeType = PropTypes.oneOfType([
+  PropTypes.shape(parentShape),
+  leafType,
+]);
+
+const propTypes = {
+  data: PropTypes.arrayOf(nodeType),
+  width: PropTypes.number,
+  height: PropTypes.number,
+  numberFormat: PropTypes.string,
+  colorScheme: PropTypes.string,
+  treemapRatio: PropTypes.number,
+};
 
 /* Modified from http://bl.ocks.org/ganeshv/6a8e9ada3ab7f2d88022 */
-function treemap(slice, payload) {
-  const div = d3.select(slice.selector);
-  const _draw = function (data, eltWidth, eltHeight, formData) {
-    const margin = { top: 0, right: 0, bottom: 0, left: 0 };
+function treemap(element, props) {
+  PropTypes.checkPropTypes(propTypes, props, 'prop', 'Treemap');
+
+  const {
+    data,
+    width,
+    height,
+    numberFormat,
+    colorScheme,
+    treemapRatio,
+  } = props;
+  const div = d3.select(element);
+  const formatNumber = d3.format(numberFormat);
+
+  function draw(data, eltWidth, eltHeight) {
+    const margin = {
 
 Review comment:
   should we pass this in as a prop? maybe in the future / not saying it should be part of this PR, just seems weird to include it, with all 0s, and not have it be set-able.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org