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

LF-4251: Add more inputs to AddSoilAmendmentProducts component #3241

Conversation

SayakaOno
Copy link
Collaborator

@SayakaOno SayakaOno commented Jun 19, 2024

Description

I rebased the branch! ⚠️ 🙏

  • Update useExpandableItem to handle empty props.
  • Update CompositionInputs and NumberInputWithSelect to
    • handle single unit. (If there are no unit options, unit field will not be registered for react-hook-form)
    • update justifyContent to centre the selected unit. (It will still be right-aligned when disabled.)
    • display error message conditionally.
  • Add unit options to nutrients. (ppm, mg/kg)
  • Add inputs for:
    • purpose
    • fertiliser type
    • moisture/dry matter contents
    • additional nutrients
  • Improve the condition to disable "add another product" button
  • Update stories

Jira link: https://lite-farm.atlassian.net/browse/LF-4251

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@SayakaOno SayakaOno added the enhancement New feature or request label Jun 19, 2024
@SayakaOno SayakaOno self-assigned this Jun 19, 2024
@SayakaOno SayakaOno force-pushed the LF-4251/Add_more_inputs_to_AddSoilamendmentProducts_component branch from ecd3192 to 373c1e7 Compare June 26, 2024 23:08
@SayakaOno SayakaOno changed the title [WIP] LF-4251: Add more inputs to AddSoilAmendmentProducts component LF-4251: Add more inputs to AddSoilAmendmentProducts component Jun 27, 2024
@SayakaOno SayakaOno marked this pull request as ready for review June 27, 2024 22:58
@SayakaOno SayakaOno requested review from a team as code owners June 27, 2024 22:58
@SayakaOno SayakaOno requested review from Duncan-Brain and kathyavini and removed request for a team June 27, 2024 22:58
Copy link
Collaborator

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

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

Looking great overall!

EDITED: NEVERMIND the request changes I think I did the same thing as before with 1:1 vs percent and thinking it was not working lol

I also thought at first it was not possible to "clear" moisture content percent back to undefined/null values but instead of using backspace I used delete and it was ok.

Duncan-Brain
Duncan-Brain previously approved these changes Jul 2, 2024
@SayakaOno
Copy link
Collaborator Author

Thank you @Duncan-Brain for reviewing!
The moisture content percent input seems to be working fine for me with both backspace and delete, but I will keep an eye on it!

I noticed that I forgot to add options for molecular_compounds_unit... I will add it to this PR, sorry 🙏

Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

Okay the form has gotten rather long + complicated but this looks really good!

I think I do mostly follow the form logic now and the relationship between the two... I think the <QuantityApplicationRate /> will need to call useFormContext() as you mentioned on that PR, and also have the namePrefix applied to all fields, correct? It shouldn't be too bad then, I think.

rules={{ required: true }}
rules={{
required: true,
validate: (value) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using this + the form isValid state is indeed quite improved (simplified) from getInvalidProductsUpdater 🙂 I like it!

Copy link
Collaborator

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

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

Looks amazing! A flakey test failed on cropVariety tests so I re-ran the test.

@Duncan-Brain Duncan-Brain added this pull request to the merge queue Jul 9, 2024
Merged via the queue into integration with commit cbdff24 Jul 9, 2024
5 checks passed
@SayakaOno
Copy link
Collaborator Author

@kathyavini Thank you for reviewing!
Yes, that's correct! useFormContext() + namePrefix should add the form values to the right place!

kathyavini added a commit that referenced this pull request Jul 11, 2024
Saw this pattern in #3241 and liked it
SayakaOno pushed a commit that referenced this pull request Jul 12, 2024
…AddSoilamendmentProducts_component

LF-4251: Add more inputs to AddSoilAmendmentProducts component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants