Page MenuHomePhabricator

Fix negative chart values, add storage for charts
ClosedPublic

Authored by epriestley on Apr 17 2019, 1:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 1:58 AM
Unknown Object (File)
Wed, Mar 27, 1:58 AM
Unknown Object (File)
Wed, Mar 27, 12:24 AM
Unknown Object (File)
Feb 3 2024, 10:13 PM
Unknown Object (File)
Jan 23 2024, 1:11 PM
Unknown Object (File)
Jan 23 2024, 10:13 AM
Unknown Object (File)
Dec 22 2023, 2:06 AM
Unknown Object (File)
Dec 3 2023, 9:28 AM
Subscribers
Restricted Owners Package

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
Branch
chart2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22613
Build 30989: Run Core Tests
Build 30988: arc lint + arc unit

Event Timeline

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
amckinley added inline comments.
src/applications/fact/storage/PhabricatorFactChart.php
56

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
src/applications/fact/storage/PhabricatorFactChart.php
56

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.