ral-facilities / inventory-management-system

Apache License 2.0
2 stars 0 forks source link

Catalogue item dialog rhf and zod #946 #958

Closed joshuadkitenge closed 1 month ago

joshuadkitenge commented 1 month ago

Description

Refactor the catalogue item dialog so that it uses react hook form and zod

Testing instructions

Add a set up instructions describing how the reviewer should test the code

Agile board tracking

closes #946

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 99.81550% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.22%. Comparing base (77be9cc) to head (d79f64b). Report is 18 commits behind head on develop.

Files with missing lines Patch % Lines
...catalogue/items/catalogueItemsDialog.component.tsx 99.79% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #958 +/- ## =========================================== - Coverage 99.23% 99.22% -0.02% =========================================== Files 72 72 Lines 16104 15994 -110 Branches 2367 2791 +424 =========================================== - Hits 15981 15870 -111 + Misses 122 120 -2 - Partials 1 4 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

joelvdavies commented 1 month ago

Also noticed that clicking edit and removing a manufacturer does this when saving image (Clear button or using backspace)

joelvdavies commented 1 month ago

Also noticed that clicking edit and removing a manufacturer does this when saving image (Clear button or using backspace)

This is still the case with backspace. I'm not sure disabling the clear button should be the fix.

joshuadkitenge commented 1 month ago

Also noticed that clicking edit and removing a manufacturer does this when saving image (Clear button or using backspace)

This is still the case with backspace. I'm not sure disabling the clear button should be the fix.

https://github.com/user-attachments/assets/aec56720-11bb-41f6-9fc5-d304c01b9e71

I have changed implementation of manufacturers autocomplete in this pr already

joelvdavies commented 1 month ago

LGTM, Just have 1 comment. I don't know if it should be addressed in a separate issue, but it would be nice to have some validation for negative numbers where they shouldn't be allowed. Could add an extra "Allowed Values" option like "Range" for Numbers properties.

image image

Both great suggestions, in fact the first one on negative numbers is part of the reason we thought using Zod for schema validation would be useful as it allows that sort of validation to defined as part of the types rather having to add it everywhere. I think we should do that at some point after this is complete (thanks for reminding us 😄). Also the allowed values should hopefully have been written in a way to allow adding another type, I believe we raised it as a possibility but they didn't require it at the time.

joshuadkitenge commented 1 month ago

LGTM, Just have 1 comment. I don't know if it should be addressed in a separate issue, but it would be nice to have some validation for negative numbers where they shouldn't be allowed. Could add an extra "Allowed Values" option like "Range" for Numbers properties.

I have created the issue for the extra validation

970