-
Notifications
You must be signed in to change notification settings - Fork 9
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
Forecast - Actual column split #3405
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3405 +/- ##
==========================================
- Coverage 81.08% 81.04% -0.04%
==========================================
Files 280 280
Lines 9805 9815 +10
Branches 454 459 +5
==========================================
+ Hits 7950 7955 +5
- Misses 1733 1736 +3
- Partials 122 124 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks for tests! I'm wondering if we should model the grass curing cell differently from forecast/actual or if it's worth the effort to include the distinction. I guess a grass curing value is an actual?
|
||
const isActual = !isNaN(params.row[actualField]) | ||
// We can disable a cell if an Actual exists or the forDate is before today. | ||
// Both forDate and today are currently in the system's time zone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is relatively simple, but could we pull this out into a helper function? Would make it easier to test and keep renderForecastCellWith
focused on the rendering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I can do that. I was also thinking the logic on line 85, setting value
might be better to put in the predictionItemValueFormatter
....it would keep formatting in the Formatter. Worth changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's a good call too
I initially thought the forecast/actual was the way to go for GC. It kind of makes sense, there is an 'actual' or 'estimated actual' and a 'forecast' as BS pointed out. Having an Actual also makes it nice and easy to disable the cells when an 'Actual' is posted. Honestly I'm not sure what the best way to go is, and what's worth the effort. Open to ideas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I think it looks good once the changes you and Conor commented on are implemented.
Ok cool, yeah I think this makes sense. If there was more divergence in the way GC works and it felt like fitting a square peg in a round hole it'd probably be worth the effort to implement a separate code path, but it seems to fit the model, at least for now. |
Quality Gate passedIssues Measures |
closes #3394
Test Links:
Landing Page
MoreCast 2.0
Percentile Calculator
MoreCast
C-Haines
FireBat
FireBat bookmark
Auto Spatial Advisory (ASA)
HFI Calculator