Page MenuHomePhabricator

Fix negative chart values, add storage for charts
ClosedPublic

Authored by epriestley on Apr 17 2019, 1:33 AM.

Details

Summary

Ref T13279. I think I'm going to fling some stuff at the wall for a bit here and hope most of it sticks, so this series of changes may not be terribly cohesive or focused. Here:

The range of the chart is locked to "[0, 105% of max]". This is trying to make a pleasing extra margin above the maximum value, but currently just breaks charts with negative values. Later:

  • I'll probably let users customize this.
  • We should likely select 0 as the automatic minimum for charts with no negative values.
  • For charts with positive values, it would be nice to automatically pick a pleasantly round number (25, 100, 1000) as a maximum by default.

We don't have any storage for charts yet. Add some. This works like queries, where every possible configuration gets a short URL slug. Nothing writes or reads this yet.

Rename fn() to css_function(). This builds CSS functions for D3. The JS is likely to get substantial structural rewrites later on, fn() was just particularly offensive.

Test Plan

Viewed a fact series with negative values. Ran bin/storage upgrade.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 17 2019, 1:33 AM
Owners added a subscriber: Restricted Owners Package.Apr 17 2019, 1:33 AM
epriestley retitled this revision from Fix negative chart values, add storage, to Fix negative chart values, add storage for charts.Apr 17 2019, 1:34 AM
epriestley requested review of this revision.Apr 17 2019, 1:34 AM
amckinley accepted this revision.Apr 17 2019, 9:54 PM
amckinley added inline comments.
src/applications/fact/storage/PhabricatorFactChart.php
57

I can see the exception above, and this is consistent with charts being immutable, but is that the plan going forward? Or is this "if you want to edit the chart, fork it", ala saved queries?

webroot/rsrc/js/application/maniphest/behavior-line-chart.js
11

Yeah, jeez, wow.

This revision is now accepted and ready to land.Apr 17 2019, 9:54 PM
epriestley added inline comments.Apr 17 2019, 10:15 PM
src/applications/fact/storage/PhabricatorFactChart.php
57

I think I'm currently planning to have some other object later which is analogous to a "NamedQuery", where you click a button like "[I like this chart]", pick a name, it saves to your list of charts you like, and you can update it later (but updating the chart part just points it at a different one of these immutable objects).

The mutable object can have a name, can have its underlying actual chart updated, can be tagged with projects, etc. So if you have something you look at in your standup every morning or something, it'd get saved as a proper chart. Maybe?

But maybe dashboard widgets are this object?

So I guess I'm not totally sure yet.

This revision was automatically updated to reflect the committed changes.