From a80426b339c4f9b42eb7ecf7b7bae6c3eef3d880 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 May 2019 07:06:14 -0700 Subject: [PATCH] Provide chart function labels over the wire instead of making them up Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions. Test Plan: {F6438872} Reviewers: amckinley Reviewed By: amckinley Subscribers: yelirekim Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20501 --- resources/celerity/map.php | 21 ++++--- src/__phutil_library_map__.php | 2 + .../fact/chart/PhabricatorChartFunction.php | 17 ++++++ .../chart/PhabricatorChartFunctionLabel.php | 56 +++++++++++++++++++ .../PhabricatorChartStackedAreaDataset.php | 15 +++-- webroot/rsrc/js/application/fact/Chart.js | 13 ++++- .../js/application/fact/ChartCurtainView.js | 10 +++- .../js/application/fact/ChartFunctionLabel.js | 35 ++++++++++++ 8 files changed, 149 insertions(+), 20 deletions(-) create mode 100644 src/applications/fact/chart/PhabricatorChartFunctionLabel.php create mode 100644 webroot/rsrc/js/application/fact/ChartFunctionLabel.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c344701d4a..d682369496 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -389,8 +389,9 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'c715c123', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '6a85bc5a', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '47a0728b', - 'rsrc/js/application/fact/Chart.js' => 'b88a227d', - 'rsrc/js/application/fact/ChartCurtainView.js' => 'd10a3c25', + 'rsrc/js/application/fact/Chart.js' => 'eec96de0', + 'rsrc/js/application/fact/ChartCurtainView.js' => '86954222', + 'rsrc/js/application/fact/ChartFunctionLabel.js' => '81de1dab', 'rsrc/js/application/files/behavior-document-engine.js' => '243d6c22', 'rsrc/js/application/files/behavior-icon-composer.js' => '38a6cedb', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => 'a17b84f1', @@ -697,8 +698,9 @@ return array( 'javelin-behavior-user-menu' => '60cd9241', 'javelin-behavior-view-placeholder' => 'a9942052', 'javelin-behavior-workflow' => '9623adc1', - 'javelin-chart' => 'b88a227d', - 'javelin-chart-curtain-view' => 'd10a3c25', + 'javelin-chart' => 'eec96de0', + 'javelin-chart-curtain-view' => '86954222', + 'javelin-chart-function-label' => '81de1dab', 'javelin-color' => '78f811c9', 'javelin-cookie' => '05d290ef', 'javelin-diffusion-locate-file-source' => '94243d89', @@ -1933,11 +1935,6 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), - 'b88a227d' => array( - 'phui-chart-css', - 'd3', - 'javelin-chart-curtain-view', - ), 'b9109f8f' => array( 'javelin-behavior', 'javelin-uri', @@ -2128,6 +2125,12 @@ return array( 'phabricator-keyboard-shortcut', 'javelin-stratcom', ), + 'eec96de0' => array( + 'phui-chart-css', + 'd3', + 'javelin-chart-curtain-view', + 'javelin-chart-function-label', + ), 'ef836bf2' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 21cd7c90d9..097bb1bb76 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2668,6 +2668,7 @@ phutil_register_library_map(array( 'PhabricatorChartFunction' => 'applications/fact/chart/PhabricatorChartFunction.php', 'PhabricatorChartFunctionArgument' => 'applications/fact/chart/PhabricatorChartFunctionArgument.php', 'PhabricatorChartFunctionArgumentParser' => 'applications/fact/chart/PhabricatorChartFunctionArgumentParser.php', + 'PhabricatorChartFunctionLabel' => 'applications/fact/chart/PhabricatorChartFunctionLabel.php', 'PhabricatorChartRenderingEngine' => 'applications/fact/engine/PhabricatorChartRenderingEngine.php', 'PhabricatorChartStackedAreaDataset' => 'applications/fact/chart/PhabricatorChartStackedAreaDataset.php', 'PhabricatorChatLogApplication' => 'applications/chatlog/application/PhabricatorChatLogApplication.php', @@ -8683,6 +8684,7 @@ phutil_register_library_map(array( 'PhabricatorChartFunction' => 'Phobject', 'PhabricatorChartFunctionArgument' => 'Phobject', 'PhabricatorChartFunctionArgumentParser' => 'Phobject', + 'PhabricatorChartFunctionLabel' => 'Phobject', 'PhabricatorChartRenderingEngine' => 'Phobject', 'PhabricatorChartStackedAreaDataset' => 'PhabricatorChartDataset', 'PhabricatorChatLogApplication' => 'PhabricatorApplication', diff --git a/src/applications/fact/chart/PhabricatorChartFunction.php b/src/applications/fact/chart/PhabricatorChartFunction.php index 5b610ae0fa..ac7ab64650 100644 --- a/src/applications/fact/chart/PhabricatorChartFunction.php +++ b/src/applications/fact/chart/PhabricatorChartFunction.php @@ -4,6 +4,7 @@ abstract class PhabricatorChartFunction extends Phobject { private $argumentParser; + private $functionLabel; final public function getFunctionKey() { return $this->getPhobjectClassConstant('FUNCTIONKEY', 32); @@ -43,6 +44,22 @@ abstract class PhabricatorChartFunction return $this; } + public function setFunctionLabel(PhabricatorChartFunctionLabel $label) { + $this->functionLabel = $label; + return $this; + } + + public function getFunctionLabel() { + if (!$this->functionLabel) { + $this->functionLabel = id(new PhabricatorChartFunctionLabel()) + ->setName(pht('Unlabeled Function')) + ->setColor('rgba(255, 0, 0, 1)') + ->setFillColor('rgba(255, 0, 0, 0.15)'); + } + + return $this->functionLabel; + } + final public static function newFromDictionary(array $map) { PhutilTypeSpec::checkMap( $map, diff --git a/src/applications/fact/chart/PhabricatorChartFunctionLabel.php b/src/applications/fact/chart/PhabricatorChartFunctionLabel.php new file mode 100644 index 0000000000..ad85c49b71 --- /dev/null +++ b/src/applications/fact/chart/PhabricatorChartFunctionLabel.php @@ -0,0 +1,56 @@ +name = $name; + return $this; + } + + public function getName() { + return $this->name; + } + + public function setColor($color) { + $this->color = $color; + return $this; + } + + public function getColor() { + return $this->color; + } + + public function setIcon($icon) { + $this->icon = $icon; + return $this; + } + + public function getIcon() { + return $this->icon; + } + + public function setFillColor($fill_color) { + $this->fillColor = $fill_color; + return $this; + } + + public function getFillColor() { + return $this->fillColor; + } + + public function toWireFormat() { + return array( + 'name' => $this->getName(), + 'color' => $this->getColor(), + 'icon' => $this->getIcon(), + 'fillColor' => $this->getFillColor(), + ); + } + +} diff --git a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php index 1683e6e268..f38ec045b1 100644 --- a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php +++ b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php @@ -131,15 +131,20 @@ final class PhabricatorChartStackedAreaDataset $events[] = $event_list; } + $wire_labels = array(); + foreach ($functions as $function_key => $function) { + $label = $function->getFunctionLabel(); + + $label->setName(pht('Important Data %s', $function_key)); + + $wire_labels[] = $label->toWireFormat(); + } + $result = array( 'type' => $this->getDatasetTypeKey(), 'data' => $series, 'events' => $events, - 'color' => array( - 'blue', - 'cyan', - 'green', - ), + 'labels' => $wire_labels, ); return $result; diff --git a/webroot/rsrc/js/application/fact/Chart.js b/webroot/rsrc/js/application/fact/Chart.js index 25703e7521..9ce50822ee 100644 --- a/webroot/rsrc/js/application/fact/Chart.js +++ b/webroot/rsrc/js/application/fact/Chart.js @@ -3,6 +3,7 @@ * @requires phui-chart-css * d3 * javelin-chart-curtain-view + * javelin-chart-function-label */ JX.install('Chart', { @@ -144,13 +145,19 @@ JX.install('Chart', { .y(function(d) { return y(d.y1); }); for (var ii = 0; ii < dataset.data.length; ii++) { + var label = new JX.ChartFunctionLabel(dataset.labels[ii]); + + var fill_color = label.getFillColor() || label.getColor(); + g.append('path') - .style('fill', dataset.color[ii % dataset.color.length]) - .style('opacity', '0.15') + .style('fill', fill_color) .attr('d', area(dataset.data[ii])); + var stroke_color = label.getColor(); + g.append('path') .attr('class', 'line') + .style('stroke', stroke_color) .attr('d', line(dataset.data[ii])); g.selectAll('dot') @@ -181,7 +188,7 @@ JX.install('Chart', { div.style('opacity', 0); }); - curtain.addFunctionLabel('Important Data'); + curtain.addFunctionLabel(label); } }, diff --git a/webroot/rsrc/js/application/fact/ChartCurtainView.js b/webroot/rsrc/js/application/fact/ChartCurtainView.js index 07e5af930e..95eb825d49 100644 --- a/webroot/rsrc/js/application/fact/ChartCurtainView.js +++ b/webroot/rsrc/js/application/fact/ChartCurtainView.js @@ -64,17 +64,21 @@ JX.install('ChartCurtainView', { return this._labelsNode; }, - _newFunctionLabelItem: function(item) { + _newFunctionLabelItem: function(label) { var item_attrs = { className: 'chart-function-label-list-item' }; var icon = new JX.PHUIXIconView() - .setIcon('fa-circle'); + .setIcon(label.getIcon()); + + // Charts may use custom colors, so we can't rely on the CSS classes + // which only provide standard colors like "red" and "blue". + icon.getNode().style.color = label.getColor(); var content = [ icon.getNode(), - item + label.getName() ]; return JX.$N('li', item_attrs, content); diff --git a/webroot/rsrc/js/application/fact/ChartFunctionLabel.js b/webroot/rsrc/js/application/fact/ChartFunctionLabel.js new file mode 100644 index 0000000000..17a943b240 --- /dev/null +++ b/webroot/rsrc/js/application/fact/ChartFunctionLabel.js @@ -0,0 +1,35 @@ +/** + * @provides javelin-chart-function-label + */ +JX.install('ChartFunctionLabel', { + + construct: function(spec) { + this._name = spec.name; + this._color = spec.color; + this._icon = spec.icon; + this._fillColor = spec.fillColor; + }, + + members: { + _name: null, + _color: null, + _icon: null, + _fillColor: null, + + getColor: function() { + return this._color; + }, + + getName: function() { + return this._name; + }, + + getIcon: function() { + return this._icon || 'fa-circle'; + }, + + getFillColor: function() { + return this._fillColor; + } + } +});