Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored forecast chart #4366

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Refactored forecast chart #4366

merged 3 commits into from
Aug 7, 2024

Conversation

jryu01
Copy link
Contributor

@jryu01 jryu01 commented Aug 6, 2024

Description

Previously, a separate top level tooltip layer used the same dataset as the statistic layer, duplicating data and doubling the number of data points in the chart spec. This change refactors the chart by placing the main line and tooltip layers under a single top layer object containing the dataset. Also, this allows additional sublayer objects, such as annotations to be added to the chart.
Example:

// Now we have 
layer: [
  // statistical layer
  {
    dataset: data,
    layer: [
      { line layer },
      { tooltip layer },
     .... 
    ]
  }
]

// Instead of
layer: [
  {
    // statistical layer
    dataset: data,
     .... 
  },
  {
    // overlay tooltip layer
    dataset: data,
     .... 
  }
  ...
]

Resolves #(issue)

@jryu01 jryu01 marked this pull request as ready for review August 6, 2024 14:00
Copy link
Member

@YohannParis YohannParis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mwdchang
Copy link
Member

mwdchang commented Aug 6, 2024

Sorry, I should have mentioned this before @jryu01 - the tooltip layer is somewhat temporary, it was something that Jamie put together pretty quickly but not exactly what we want to have.

The ideal "toolip" would be a rule-based tooltip, rather than the currently point/cursor based tooltip. A rule-based tooltip will have a better options for selection heuristics - whereas right now we artificially inflate the stroke width to make things "easier" to select.
But moreover, the tooltip wouldn't be restricted to just the statistical data, but others like values from ground-truth layer as well. (ground-truth and stat layers are not necessarily aligned ... so we also haven't done this part to merge them together).

@jryu01
Copy link
Contributor Author

jryu01 commented Aug 7, 2024

@mwdchang That's good to know! Yeah we may need further work to have better tooltip. For this change, the main focus was to restructure the layers so that the annotations can be added to the chart easily for (#4374). I just relocated the tooltip in the process to avoid data duplication, and this doesn't change the current behaviour of the chart.

@mwdchang mwdchang merged commit 8df83ad into main Aug 7, 2024
5 checks passed
@mwdchang mwdchang deleted the forecast-chart-refactor branch August 7, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants