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

Morecast: Validation on editable fields #3919

Merged
merged 23 commits into from
Sep 16, 2024
Merged

Conversation

conbrad
Copy link
Collaborator

@conbrad conbrad commented Sep 11, 2024

Adds an optional validator function for IndeterminateField that gets called in preProcessEditCellProps callback for cells in edit mode to set the error prop if validation doesn't pass for the forecast parameter. The same validator function is passed down to cells in view mode to do the same thing.

Adds the EditInputCell component as the render component for cells in edit mode that gets rendered by the renderEditCell callback on the GridColDef. EditInputCell props include an empty or non-empty error string that drives the visibility of the tooltip and red border.

Similar behaviour for the renderCell callback for cells in view mode, except it doesn't receive the error string in props so it calls validator itself. This is to keep the same behaviour whether the user is actively editing or not and the current value is invalid.

Opted to prevent edits from committing locally when invalid so that invalid values aren't stored in drafts.

Closes #3571

Test Links:

Landing Page
MoreCast
Percentile Calculator
C-Haines
FireBat
FireBat bookmark
Auto Spatial Advisory (ASA)
HFI Calculator

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 91.13924% with 7 lines in your changes missing coverage. Please review.

Project coverage is 81.35%. Comparing base (b7d7bc0) to head (d54cfa1).

Files with missing lines Patch % Lines
...features/moreCast2/components/ColumnDefBuilder.tsx 55.55% 4 Missing ⚠️
.../features/moreCast2/components/MoreCast2Column.tsx 89.47% 1 Missing and 1 partial ⚠️
...b/src/features/moreCast2/slices/validInputSlice.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3919      +/-   ##
==========================================
+ Coverage   81.31%   81.35%   +0.04%     
==========================================
  Files         294      298       +4     
  Lines       11362    11421      +59     
  Branches      526      546      +20     
==========================================
+ Hits         9239     9292      +53     
- Misses       2089     2094       +5     
- Partials       34       35       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dgboss
Copy link
Collaborator

dgboss commented Sep 12, 2024

Could we get rid of the new blue border when editing a cell?
image

@conbrad conbrad marked this pull request as ready for review September 12, 2024 22:53
@conbrad conbrad requested a review from dgboss September 12, 2024 23:53
@dgboss
Copy link
Collaborator

dgboss commented Sep 12, 2024

It's looking really good!
I noticed that validation errors/tooltip doesn't persist when you go from the weather parameter tabs to the Forecast Summary tab. Realistically this probably isn't an issue as I would expect a forecaster to correct validation errors as they enter them.

@dgboss
Copy link
Collaborator

dgboss commented Sep 12, 2024

Oh, should we add validation for the grass curing fields as well?

@conbrad
Copy link
Collaborator Author

conbrad commented Sep 13, 2024

It's looking really good! I noticed that validation errors/tooltip doesn't persist when you go from the weather parameter tabs to the Forecast Summary tab. Realistically this probably isn't an issue as I would expect a forecaster to correct validation errors as they enter them.

It's looking really good! I noticed that validation errors/tooltip doesn't persist when you go from the weather parameter tabs to the Forecast Summary tab. Realistically this probably isn't an issue as I would expect a forecaster to correct validation errors as they enter them.

Nice catch! I adjusted the code to share the validation logic between edit and view mode cells: 59a301e

@brettedw
Copy link
Collaborator

Looks great, nice work! Only comment would be same as Darren's

Oh, should we add validation for the grass curing fields as well?

@brettedw
Copy link
Collaborator

Just noticed there's a number of code smells. Unused imports in a few cases, otherwise just complaining about using optional chain expressions. I'm fine either way

@conbrad
Copy link
Collaborator Author

conbrad commented Sep 16, 2024

Just noticed there's a number of code smells. Unused imports in a few cases, otherwise just complaining about using optional chain expressions. I'm fine either way

Thanks fixed, part of it was a bunch of duplicated code between forecast and grass cure cells that I deduped in a shared ValidatedForecastCell component.

Copy link
Collaborator

@brettedw brettedw left a comment

Choose a reason for hiding this comment

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

Looks great to me! Nice work!

Copy link
Collaborator

@dgboss dgboss left a comment

Choose a reason for hiding this comment

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

This is great! Only one tiny nit that could be addressed in a followup PR. The tooltip position becomes a little wonky when toggling between the Forecast Summary tab and the other tabs. Definitely not worth holding up the PR for.

Copy link

sonarcloud bot commented Sep 16, 2024

@conbrad conbrad requested a deployment to production September 16, 2024 18:35 Abandoned
@conbrad conbrad merged commit b541b68 into main Sep 16, 2024
24 of 25 checks passed
@conbrad conbrad deleted the task/validate-editing-feedback branch September 16, 2024 18:51
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.

MoreCast: Range limits on weather values
3 participants