Flesh out D3 time series charts #657

2024-04-20T09:06:15+08:00

josefarias commented

2024-04-20 09:06:15 +08:00

(Migrated from github.com)

Depends on https://github.com/maybe-finance/maybe/pull/651 which might make this hard to review because it includes changes from that PR as well. Sorry about that!

This PR consolidates our trendline and line-chart Stimulus controllers into a single time-series-chart controller. These controllers weren't immediately compatible, so this also cleans things up by identifying the common patterns and abstracting them into reusable methods, all inside the same stimulus controller.

The Figma designs don't have change data for percent values:

I think the user loses information in this case. So I kept the scalar and percent change portion of the tooltip. If we want to remove it, it's a matter of simply including a conditional in the tooltip template method. But percent, integer, and money types are all supported by this PR in any case.

Finally, my preview screenshots were taken before I realized we'd like to show the x-axis labels as day month year. That part of the feature is not pictured, but it is implemented.

Demo

https://github.com/maybe-finance/maybe/assets/31393016/04eaec59-dc1c-4834-b033-32c35269dbdc

Previews

EDIT: Whoops! Looks like we don't want tooltips on trendlines. I don't actually mind it though? Seems useful to have. In any case, turning them off is a simple bool param away. Let me know!

Trendline Trends

-ve Trend

+ve Trend

Flat Trend

Liability -ve Trend

Percentage Trend

Empty State

Requirements

Functional Requirements

Currently, we have a trendline_controller.js and a line_chart_controller.js. We should consolidate these into a single line_chart_controller that is configurable. Options will include:
- Hide or display x-axis labels ("trendlines" have no axis) - show by default
- Enable or disable tooltips ("trendlines" have no tooltip) - enable by default
The chart controller will always receive a TimeSeries.new([]).to_json value, which can be an instance of Money or a regular integer (this is already implemented). As part of this issue, we need to make sure the chart can handle both Money values and regular integers (including percentages) in the tooltip.
When the chart is supplied with a single value or an "empty" Time Series (i.e. TimeSeries.new([])), it should display the "empty" state design.

Design Requirements

Implement gradient underneath the line (very subtle, see designs)
Ensure padding and spacing matches designs
When the overall trend of the series that is passed to the line chart is "bad" (decreasing for an asset, increasing for a liability), the chart should go from a green color to a red color.
When hovering over a data point in the chart, the line should highlight for dates prior to it, and the line after it should be grayed out

/claim #643

Depends on https://github.com/maybe-finance/maybe/pull/651 which might make this hard to review because it includes changes from that PR as well. Sorry about that! --- This PR consolidates our `trendline` and `line-chart` Stimulus controllers into a single `time-series-chart` controller. These controllers weren't immediately compatible, so this also cleans things up by identifying the common patterns and abstracting them into reusable methods, all inside the same stimulus controller. The Figma designs don't have change data for percent values: ![Figma design](https://github.com/maybe-finance/maybe/assets/31393016/66ceb7b3-8e6f-445b-81a7-30faa3f13c31) I think the user loses information in this case. So I kept the scalar and percent change portion of the tooltip. If we want to remove it, it's a matter of simply including a conditional in the tooltip template method. But percent, integer, and money types are all supported by this PR in any case. Finally, my preview screenshots were taken before I realized we'd like to show the x-axis labels as `day month year`. That part of the feature is not pictured, but it is implemented. ## Demo https://github.com/maybe-finance/maybe/assets/31393016/04eaec59-dc1c-4834-b033-32c35269dbdc ## Previews ### Trendline tooltip EDIT: Whoops! Looks like we don't want tooltips on trendlines. I don't actually mind it though? Seems useful to have. In any case, turning them off is a simple bool param away. Let me know! ![Trendline with tooltip](https://github.com/maybe-finance/maybe/assets/31393016/9f57ec6e-3170-4e95-bd5f-99ad78d5e6b5) ### Trendline Trends ![Negative Trendline](https://github.com/maybe-finance/maybe/assets/31393016/87ce918b-8e3c-411a-9930-0bfb812abae1) ![Positive Trendline](https://github.com/maybe-finance/maybe/assets/31393016/6528b33e-1473-435d-955f-064a1b97dbd8) ![Flat Trendline](https://github.com/maybe-finance/maybe/assets/31393016/b1926817-e580-46da-97bd-4dc201af1a8a) ### -ve Trend ![Negative trend](https://github.com/maybe-finance/maybe/assets/31393016/b7ef15bb-52a2-4f33-88bd-e0b3ea81c236) ### +ve Trend ![Positive Trend](https://github.com/maybe-finance/maybe/assets/31393016/7e083cbe-17d5-4753-af97-69c379bfdae6) ### Flat Trend ![Flat Trend](https://github.com/maybe-finance/maybe/assets/31393016/dc2cfc0a-1c07-4f48-9e57-e4b622b91165) ### Liability -ve Trend ![Negative trend where the favorable direction is down](https://github.com/maybe-finance/maybe/assets/31393016/c9417697-534c-4e23-b378-8645c47fa340) ### Percentage Trend ![Percentage Trend](https://github.com/maybe-finance/maybe/assets/31393016/05919e59-8e7e-4c06-8977-4819ea0a7654) ### Empty State ![Empty State](https://github.com/maybe-finance/maybe/assets/31393016/8aa30c0c-722d-4242-8ee4-386aa4a34066) ## Requirements ### Functional Requirements * [x] Currently, we have a `trendline_controller.js` and a `line_chart_controller.js`. We should consolidate these into a single `line_chart_controller` that is configurable. Options will include: * [x] Hide or display x-axis labels ("trendlines" have no axis) - show by default * [x] Enable or disable tooltips ("trendlines" have no tooltip) - enable by default * [x] The chart controller will always receive a `TimeSeries.new([]).to_json` value, which can be an instance of `Money` or a regular integer (this is already implemented). As part of this issue, we need to make sure the chart can handle both `Money` values and regular integers (including percentages) in the tooltip. * [x] When the chart is supplied with a **single value** or an "empty" Time Series (i.e. `TimeSeries.new([])`), it should display the "empty" state design. ### Design Requirements * [x] Implement gradient underneath the line (very subtle, see designs) * [x] Ensure padding and spacing matches designs * [x] When the _overall_ trend of the series that is passed to the line chart is "bad" (decreasing for an asset, increasing for a liability), the chart should go from a green color to a red color. * [x] When hovering over a data point in the chart, the line should highlight for dates prior to it, and the line after it should be grayed out /claim #643

zachgoll commented

2024-04-20 21:14:59 +08:00

(Migrated from github.com)

@josefarias no worries on the extra changes at all. I've read through them closely enough to distinguish between those changes and the D3 changes.

I have some local work in-progress that plays off #651 to simplify all our nil checks and reworks some of the tests to simplify requirements. Unfortunately I don't have the availability to open that up today for collaboration, so you can go ahead and assume those changes will be independent of the bounty.

I'll leave a comment on the main issue, but I'll be available to review bounties late tomorrow evening or first thing Monday morning.

@josefarias no worries on the extra changes at all. I've read through them closely enough to distinguish between those changes and the D3 changes. I have some local work in-progress that plays off #651 to simplify all our `nil` checks and reworks some of the tests to simplify requirements. Unfortunately I don't have the availability to open that up today for collaboration, so you can go ahead and assume those changes will be _independent_ of the bounty. I'll leave a comment on the main issue, but I'll be available to review bounties late tomorrow evening or first thing Monday morning.

syedbarimanjan commented

2024-04-21 11:21:32 +08:00

(Migrated from github.com)

Great work on this!

You should also add a configuration option which will be used to tell the chart that it should render only precentage values in the tooltip as shown in the design(the last card) as per this comment:
https://github.com/maybe-finance/maybe/pull/652#issuecomment-2066668131

Great work on this! You should also add a configuration option which will be used to tell the chart that it should render only precentage values in the tooltip as shown in the design(the last card) as per this comment: https://github.com/maybe-finance/maybe/pull/652#issuecomment-2066668131

josefarias commented

2024-04-22 02:12:02 +08:00

(Migrated from github.com)

Thanks @syedbarimanjan! And thanks for the heads up!

I added a note about that percentage-only design in the PR description. It's easy to add, but I want to make sure that's what we actually want, since the user loses information in that case. Let me know @zachgoll!

This is ready for specific feedback.

Thanks @syedbarimanjan! And thanks for the heads up! I added a note about that percentage-only design in the PR description. It's easy to add, but I want to make sure that's what we actually want, since the user loses information in that case. Let me know @zachgoll! This is ready for specific feedback.

👍 2

zachgoll commented

2024-04-22 20:47:31 +08:00

(Migrated from github.com)

@josefarias looks awesome, this is great! A few responses below:

RE: tooltips - let's keep them, I kind of like them too and as you mention, very easy to remove.
RE: percentage - I think omitting this information could be a net positive to the user and help avoid the cognitive overhead of trying to think about a "percent change of a percent". We're showing "percent change of percent" over the entire series (in the header) which seems like enough information for the user IMO.

Also, I went ahead and merged in the time series refactor, but hadn't realized you made a few incremental changes on that here. Let me know if you'd like me to revert that commit so you don't have to resolve the conflicts. My main reasoning for merging that was for a cleaner history / documentation purposes.

@josefarias looks awesome, this is great! A few responses below: - RE: tooltips - let's keep them, I kind of like them too and as you mention, very easy to remove. - RE: percentage - I think omitting this information could be a net positive to the user and help avoid the cognitive overhead of trying to think about a "percent change of a percent". We're showing "percent change of percent" over the entire series (in the header) which seems like enough information for the user IMO. Also, I went ahead and merged in the time series refactor, but hadn't realized you made a few incremental changes on that here. Let me know if you'd like me to revert that commit so you don't have to resolve the conflicts. My main reasoning for merging that was for a cleaner history / documentation purposes.

josefarias commented

2024-04-22 20:50:49 +08:00

(Migrated from github.com)

Thanks @zachgoll! Totally, makes sense to merge that other PR first. Conflicts should be no problem. I'll resolve them (and implement that bit about the percent change) once I'm online in a couple of hours if that works for you.

👍 1

zachgoll (Migrated from github.com) approved these changes 2024-04-22 21:30:36 +08:00

zachgoll (Migrated from github.com) left a comment

Looked through everything and think we're all set with this!

One of the things I was contemplating as an improvement to the TimeSeries class is to allow for individual data points to have trend value equal to nil. This better aligns with the domain as there is no such thing as a "null trend"—it either exists (there are 2 data points to compare) or doesn't. I think this will clean up several of those nil checks you had mentioned. That will require a few small checks in the D3 controller for the presence of the trend, but I'll handle that all in one PR. Just wanted to let you know of those expected changes.

Looked through everything and think we're all set with this! One of the things I was contemplating as an improvement to the `TimeSeries` class is to _allow_ for individual data points to have `trend` value equal to `nil`. This better aligns with the domain as there is no such thing as a "null trend"—it either exists (there are 2 data points to compare) or doesn't. I think this will clean up several of those `nil` checks you had mentioned. That will require a few small checks in the D3 controller for the presence of the trend, but I'll handle that all in one PR. Just wanted to let you know of those expected changes.

app/javascript/controllers/time_series_chart_controller.js

						
				@@ -0,0 +1,518 @@

				import { Controller } from "@hotwired/stimulus"

zachgoll (Migrated from github.com) commented

2024-04-22 21:03:29 +08:00

Very small naming nitpick that I've been thinking about in regards to the time series classes in general:

What do you think about introducing the concept of "Data" and "Datum", where data is a collection of datum and datum reads as a "piece of data"?

Very small naming nitpick that I've been thinking about in regards to the time series classes in general: What do you think about introducing the concept of "Data" and "Datum", where `data` is a collection of `datum` and `datum` reads as a "piece of data"?

josefarias commented

2024-04-22 23:41:05 +08:00

(Migrated from github.com)

@zachgoll changes have been made and conflicts resolved.

That's interesting about the nil values in trends. I think that makes sense but I'd have to see the code to be sure. I think we're missing a duck type or two in some places.

Like, ideally we'd be able to call i.e. .value or .trend on anything and have it just work in both JS and Ruby. Regardless of the underlying data being Money, Numeric, BigDecimal, or NilClass.

In the case of values, I think we can handle the normalization on the ruby side and have both integers and Money instances have the same shape, then include currency at the top level. So:

{
  date: Date.new,
  value: always an int,
  currency: sometimes nil
}

And the presence or absence of currency will tell JS what kind of scalar we're dealing with.

I'm also unsure if the presence of nil values in a time series points to a problem with data integrity — how can we build a continuous time series if there's missing data points? I suspect this is best handled further upstream where missing points are intelligently assigned a value so trend calculations don't have to be aware of a value being potentially missing. But that's just a hunch, and an ill-informed one at that. Need more familiarity with the domain.

I'm conflating multiple parts of the codebase — the BigDecimal check only happens in Money, the #value shape discrepancy only happens in TimeSeries::Value (I think), and the nil values thing only happens in trends.

I think this is all good to keep in mind as we build. No need to solve for everything right now. The right abstractions will present themselves, but I think they might be in-line with the above.

@zachgoll changes have been made and conflicts resolved. That's interesting about the `nil` values in trends. I think that makes sense but I'd have to see the code to be sure. I think we're missing a duck type or two in some places. Like, ideally we'd be able to call i.e. `.value` or `.trend` on anything and have it just work in both JS and Ruby. Regardless of the underlying data being Money, Numeric, BigDecimal, or NilClass. In the case of values, I think we can handle the normalization on the ruby side and have both integers and Money instances have the same shape, then include currency at the top level. So: ```rb { date: Date.new, value: always an int, currency: sometimes nil } ``` And the presence or absence of `currency` will tell JS what kind of scalar we're dealing with. I'm also unsure if the presence of `nil` values in a time series points to a problem with data integrity — how can we build a continuous time series if there's missing data points? I suspect this is best handled further upstream where missing points are intelligently assigned a value so trend calculations don't have to be aware of a value being potentially missing. But that's just a hunch, and an ill-informed one at that. Need more familiarity with the domain. I'm conflating multiple parts of the codebase — the BigDecimal check only happens in `Money`, the `#value` shape discrepancy only happens in `TimeSeries::Value` (I think), and the `nil` values thing only happens in trends. I think this is all good to keep in mind as we build. No need to solve for everything right now. The right abstractions will present themselves, but I think they might be in-line with the above.

zachgoll commented

2024-04-23 01:43:55 +08:00

(Migrated from github.com)

@josefarias

In the case of values, I think we can handle the normalization on the ruby side and have both integers and Money instances have the same shape, then include currency at the top level.

Agree with this.

I'm also unsure if the presence of nil values in a time series points to a problem with data integrity

I was mostly referring to the following cases, but I suppose these could be handled as special cases upstream and we just pass the same value for current and previous. When I say "there can't be a nil Trend" I'm mostly referring to the idea that regardless of who fills in the previous value (whether that be Trend or upstream), there must be a previous value to be considered a "Trend" (which I think you're also saying in regards to data integrity here).

First value in series
Series of length == 1

I think we're missing a duck type or two in some places.

100% agree. Between TimeSeries, Trend, and ValueGroup, I think there's a slight tweak somewhere to be made that will allow for a lot more flexibility.

@josefarias > In the case of values, I think we can handle the normalization on the ruby side and have both integers and Money instances have the same shape, then include currency at the top level. Agree with this. > I'm also unsure if the presence of nil values in a time series points to a problem with data integrity I was mostly referring to the following cases, but I suppose these _could_ be handled as special cases upstream and we just pass the same value for `current` and `previous`. When I say "there can't be a `nil` Trend" I'm mostly referring to the idea that regardless of _who_ fills in the `previous` value (whether that be `Trend` or upstream), there _must_ be a `previous` value to be considered a "Trend" (which I think you're also saying in regards to data integrity here). - First value in series - Series of length == 1 > I think we're missing a duck type or two in some places. 100% agree. Between `TimeSeries`, `Trend`, and `ValueGroup`, I think there's a slight tweak somewhere to be made that will allow for a lot more flexibility.

👍 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#657

				`@@ -0,0 +1,518 @@`
				`import { Controller } from "@hotwired/stimulus"`