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-3951: Show water calculator with wild crops #3280

Open
wants to merge 4 commits into
base: integration
Choose a base branch
from

Conversation

Joaquin-M2
Copy link
Collaborator

@Joaquin-M2 Joaquin-M2 commented Jun 27, 2024

Description

Users were not able to open the "Water calculator" (Create a task > Irrigation task) if the water measurement method was "Depth".

It is expected that the Calculate Water Usage link will be disabled (+ a warning) for wild crops if the measure of water use is set to Depth:

image

image

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

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • 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

@Joaquin-M2 Joaquin-M2 added the bug Something isn't working label Jun 27, 2024
@Joaquin-M2 Joaquin-M2 requested review from a team as code owners June 27, 2024 06:21
@Joaquin-M2 Joaquin-M2 requested review from Duncan-Brain and kathyavini and removed request for a team June 27, 2024 06:21
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.

Thank you for the fix and for your patience with our slow reviews around the soil amendment release!

I'll bring this ticket up with our designer during our weekly meeting on Wednesday (and I'll send you an invite in case you would like to join as well -- totally optional of course!)

The ticket description as written:

And the user is able to make the calculation both for “Volume” or “Depth” method.

doesn't make sense because there is no depth calculation that can be done without location area, and so the calculator doesn't calculate anything:

Screenshot 2024-07-29 at 3 40 24 PM

In soil amendment we went the route of prohibiting pin-based wild crops and I wonder if we might end up doing something similar here. If not, I think the way it works on your branch is good! It just needs a slight tweak as noted below, but let's discuss first with Loïc about the calculator opening + but not calculating.

@@ -197,7 +197,7 @@ export default function PureIrrigationTask({
const waterCalculatorDisabled =
!showWaterUseCalculatorModal ||
!measurement_type ||
(measurement_type === 'DEPTH' && !location);
(measurement_type === 'DEPTH' && !(location || getValues().show_wild_crop));
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 form value works on task creation, but if the user wants to open the calculator when they complete the task, show_wild_crop won't be available -- I think it's unique to creation and maybe something else like pinCoordinates would have to control the logic for task completion?

@Joaquin-M2
Copy link
Collaborator Author

@kathyavini I have updated the description of the PR considering what we talked about during the Dev-Design meeting from last week. I talked to Loïc on Slack and I got his confirmation about how it should look like.

On the other hand, you will see in the last commits that I've used inline styling for the additional jsx. I just tried to be consistent with the old code, but if you think I should move that into its own module.scss file please let me know.

So both the text and the icon are aligned without needing to set the margin specifically on each element.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants