LF-4249 Create <QuantityApplicationRate /> component

[kathyavini]

Description

This creates the <QuantityApplicationRate /> component. The PR includes:

• two new custom measures and database units for application rate, as these are not units included within the convert-units package
• Small revisions to <NumberInput /> and <Unit /> to pass 1) label styles, 2) additional onChange callbacks to Unit
• Storybook story for isolated component
• Updates to the parent component <SoilAmendmentProductCard /> and its Storybook story to integrate the QAR component

Not included:

• play tests for the new kind of <Unit />; this was merely due to lack of time. I could revisit if the team thinks it's a good next priority
• form value handling according to the ultimate structure of the form this will be embedded in -- @SayakaOno I wasn't sure what this was going to look like exactly and the relationship of the two sets of form fields (product and task product)... will they be wrapped in one <FormProvider />? This calls useForm() in the component which is just an interim solution. Please let me know if should refactor to useFormContext(), and please also feel free to adjust anything to integrate with the other half of task product! 🙏

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

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

[kathyavini]

Thanks @navDhammu, that's a good idea! I also like that UX better than applying onBlur.

(Sorry @SayakaOno for the invalidated review 🙇 )

[navDhammu]

I'm not sure if this is the intended behavior as I don't have full context of this feature, but If percent_of_location is 0 then the || operator here would default it to 100. If the user enters 0 for the percent then is it just disregarded and assumed that 100% of location should be ammended?

[kathyavini]

Kind of -- it's not allowed to be zero, so if the user enters zero specifically, onBlur it will get set to a very small minimum. I don't think we want to have the application area ever calculated as zero is why I'm not too concerned if the percentage is temporarily at 100% before it gets bumped to the minimum.

[navDhammu]

Isn't it possible to do this in the change handler for Unit?

<Unit onChangeUnitOption={() => updateApplicationRate()} />

If it works then I think its cleaner and potentially more performant than useEffect. Same with quantity update below.

[kathyavini]

Hmm, @Duncan-Brain had the exact same idea. To get that to work, the handler would have to take in and use the updated unit only in the onChangeUnitOption case, because that callback is called before the database unit is updated while the onInputChange handler is triggered afterwards (the useEffect is also triggered afterwards).

I thought the added complexity added to the handler wouldn't be worth it, but I think I also overestimated how much code has to be changed. Since the useEffect pattern is unintuitive enough for this to have come up twice now, I'll modify the handler.

[navDhammu]

Is this useEffect and state for previewStrings necessary? If not then I think it can be just calculated on render and if doing the calculation on every render is expensive then useMemo can be used?

[kathyavini]

Ummm, it wasn't about expensive calculation, but rather to freeze the value of the preview string unit to the user-selected database value (OR the database default if the user-selected value was in the wrong system), while the application area unit should adapt dynamically based on the calculation. There might be a better way but I think it's kind of low on the priority list right now as something to optimize.

[navDhammu]

I see what you mean, maybe I'm missing something but I was curious why not just compute the previewStringValue and unit without using state? For example in the component render, just do

const previewStringValue = total_area &&
   roundToTwoDecimal(
     convertFn(location_area[system], total_area, location_area.databaseUnit, unit.value),
   );

and pass that as a prop to AreaApplicationSummary

My reasoning for this is that with the useEffect, its triggering an unnecessary extra render. For instance, first render prevString is null, then immediately it renders again from the useEffect with setState, and also the same thing if the useEffect dependencies change.

[kathyavini]

In your code example above, where are you getting the value of unit from? Still from watching the value of application_area_unit?

[navDhammu]

Yes unit is exactly the same as its defined:

const unit =
   total_area_unit && location_area[system].units.includes(total_area_unit)
     ? getUnitOptionMap()[total_area_unit]
     : application_area_unit;

[kathyavini]

So I'm not sure I love the idea of the unit + value calculations being re-run on every render, but as I said above the string value + unit need to be frozen. If written like this, when the application_area_unit changes (e.g. you can test it in the Storybook story for imperial by setting the percentage to a very low value, and triggering the unit shift from ac > ft); the string will update as well. This is what I'm trying to prevent.

Ideally I would like to memoize the calculation and exclude application_area_unit from the dependencies, but, as you mentioned, it's initially undefined.

Maybe a ref instead of component state would have been a cleaner way to make sure the values are only set one time, though?

[navDhammu]

I’m not sure about ref but I tried this out on my local and was able to achieve the desired result by memoizing with useMemo (let me know if it doesn't work). Now as you mentioned it needs to be frozen and that’s where the issue with using application_area_unit as a dependency becomes because when it changes to a different unit the calculations are re-run with the new unit value which changes the UI string.

So what I did is turn the dependency into a boolean, it only runs when application_area_unit changes from falsy to truthy (or vice versa). So it doesn’t run when application unit changes, lets say from ‘ac’ to ‘ft-2’ because both are truthy.

Here's the code I used:

  const previewStrings = useMemo(() => {
    let previewStringUnit;
    let previewStringValue;

    if (application_area_unit) {
      previewStringUnit =
        total_area_unit && location_area[system].units.includes(total_area_unit)
          ? getUnitOptionMap()[total_area_unit]
          : application_area_unit;

      previewStringValue =
        total_area &&
        roundToTwoDecimal(
          convertFn(
            location_area[system],
            total_area,
            location_area.databaseUnit,
            previewStringUnit.value,
          ),
        );
    }

    return { previewStringUnit, previewStringValue };
  }, [!!application_area_unit]);

[kathyavini]

Mmmm I love the boolean value in dependency array! That is exactly what I wanted 😍

We won't be making any further changes in this PR to avoid merge conflicts with #3294 but I would like to adopt it there.

[navDhammu]

Also just to mention the useEffect solution works fine so its not necessary to change it. I was just pointing out what I know and from reading the react docs, the exact same thing is mentioned in this page: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state

Basically whenever something can be computed during a components render, then it probably shouldn't be in state. This is more like best practices and not a hard rule though.

[Duncan-Brain]

Looks great!

I know this will approval be cleared but there are some conflicts now -- and if you want it still might be nice to have default values in the readonly story for Page/AddSoilAmendmentProducts

[kathyavini]

@Duncan-Brain and @SayakaOno I think this branch should now work and populate with correct area(s) for add task and complete task views -- and should continue to work 🤞 with the changes you showed today @SayakaOno for complete.

[navDhammu]

Sorry for nitpicking 😁 but this can be simplified to updatedUnit?.value || 'kg/m2'. The isWeight check is already done at the top in the if condition. Same for else if below.

[kathyavini]

Oh thank you, I will change it!

Oh dear, and there is actually a logical bug in that switch to handler though so that was definitely worth taking a look at.

[kathyavini]

Hmm, unfortunately I don't think it's going to work. As we are on quite a time crunch I'm going to just revert the handler change, but the changes needed to get the calculations to work like this are actually quite a lot.

For instance, the updateUnit being passed by the onChangeUnitOption handler to updateApplicationRate is going to be in e.g. lbs, let's say for an imperial farm. But the value being pulled by getValue() is in 'kg/m2'.

I don't think there is clean logic for relating a change of lb --> oz on a metric-based hidden input value without re-writing Unit, which... doesn't look worth it for avoiding two useEffects. So I'll return to the original method 😓

[navDhammu]

I think that's just the side effect of having too much logic inside of useEffects in Unit. Actually re-writing Unit might not be a bad idea because if we're going to be using NumberInput going forward (with the localized numbers), then it probably makes sense to have the numbers in Unit behave the same way. Otherwise you have a situation with Unit and NumberInput on the same page behaving differently (commas and periods).

[SayakaOno]

Looks great!
Thank you so much for adjusting your code to work with mine. It works perfectly!

[SayakaOno]

I think you meant props.productId 😄

[kathyavini]

I absolutely did! I was only using this to pass to the <Collapse /> id, and since it was functioning with the fixed string, I think taking in productId isn't necessary for this component at all. @Duncan-Brain in the other PR I will remove the productId prop.

[SayakaOno]

If you remember, this is what I referred to! I was able to use UseFormReturn without a generic type parameter!

[kathyavini]

@Duncan-Brain I think this is good to merge. After cleaning up the diff on #3294 and undrafting I will make these changes on that branch

• remove the productId prop
• use @navDhammu's useMemo() solution for the previewString
• there is also a bug I need to fix where the the total_area calculation is not properly re-triggered when the user goes back in the task flow and re-selects a different set of areas (noticed it looking at the useMemo dependency array)