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-4545 Adjust read only task view for movement tasks #3552

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

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Nov 27, 2024

Description

This PR completes the readonly task view for animal movement tasks. Modifications:

  • The readonly animal inventory should be shown in TaskReadOnly
  • The task details component (PureMovementTask) was updated for compatibility with the readonly view
  • The incoming (GET) task data needed adjust to be shown in TaskReadOnly (the animal inventory) and PureMovementTask (the movement purpose)
    • As a reminder, the animals + animal batches are top level task properties while the purpose is nested under animal_movement_task (incoming from the backend) / movement_task (outgoing towards backend, and adjusted in saga)

Although some of the work of complete (e.g. putting the task details into StepOne and common data formatting) has been added to this branch, please consider this branch as only for populating readonly. -- edit/complete will be done on the other branch.

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

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

@kathyavini kathyavini self-assigned this Nov 27, 2024
@kathyavini kathyavini requested review from a team as code owners November 27, 2024 21:56
@kathyavini kathyavini requested review from antsgar and Duncan-Brain and removed request for a team November 27, 2024 21:56
@kathyavini kathyavini marked this pull request as draft November 27, 2024 21:56
@kathyavini kathyavini added the enhancement New feature or request label Nov 27, 2024
Base automatically changed from LF-4479/Update_Task_saga_to_account_for_new_animal_movement_task to integration November 28, 2024 16:27
@kathyavini kathyavini removed the Blocked label Dec 3, 2024
@kathyavini kathyavini marked this pull request as ready for review December 3, 2024 17:55
@kathyavini kathyavini added the new translations New translations to be sent to CrowdIn are present label Dec 3, 2024
@@ -1952,7 +1952,7 @@
"ADD_TASK_FLOW": "task creation",
"AMOUNT_TO_ALLOCATE": "Amount to allocate",
"ANIMAL_MOVING_TO_LOCATION": "Moving to:",
"ANIMAL_MOVEMENT_EXPANDING_SUMMARY_TITLE": "See detail list of animals to move",
"ANIMAL_MOVEMENT_EXPANDING_SUMMARY_TITLE": "Animals in this task",
Copy link
Collaborator Author

@kathyavini kathyavini Dec 3, 2024

Choose a reason for hiding this comment

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

The original string had just gone up to CrowdIn less than an hour ago, so I went ahead and manually adjusted this on CrowdIn (so translators will not have to translate the now irrelevant string).

Since this is already on CrowdIn, I will remove the "new translations" tag from this branch.

@kathyavini kathyavini removed the new translations New translations to be sent to CrowdIn are present label Dec 3, 2024
Per yesterday's discussion about animals on custom tasks, it will not be task-type defined
Copy link
Collaborator

@SayakaOno SayakaOno 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!!

I had a similar bug in the animal read-only view where "uses" are not shown right after logging in. (it's not very useful, but here is the commit for reference) I think we need to render the purposes after the options are fetched here too!

@kathyavini
Copy link
Collaborator Author

@SayakaOno thank you so much for taking on more than your share of reviews 🙇❤️ And thank you for the heads up about the purposes bug!! 🙏

Since I set the purposes in the component, my impulse would be to just add the options to the setting dependency array (I'll push commit of which one I mean), so it will re-run once the options go from empty array to populated. Do you think there is a disadvantage to doing it that way vs delaying render?

@SayakaOno
Copy link
Collaborator

@kathyavini Ah yes, you're calling setValue, so that works perfectly! I was thinking about cases where defaultValue is used :)

SayakaOno
SayakaOno previously approved these changes Dec 3, 2024
Copy link
Collaborator

@antsgar antsgar 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!! 🙌

Only one minor comment that is a more generic change (not only applicable to this screen). I think we shouldn't show the Remove button on the pills and the dropdown indicator when the field is disabled (for the multiselect component on Purposes here). We could hide them easily by editing the styles in the component

  multiValueRemove: (provided, state) => ({
    ...
    display: state.isDisabled ? 'none' : 'block',
  }),
  multiValue: (provided, state) => ({
    ...
    padding: state.isDisabled ? '0 18px 0 12px' : '0 4px 0 12px'
  }),
  dropdownIndicator: (provided, state) => ({
    ...
    display: state.isDisabled ? 'none' : 'block',
  }),

@kathyavini
Copy link
Collaborator Author

@antsgar oh that's a very good point -- much better!!

Unrelated to anything, I kind of love writing CSS in TS in like this 😂

@kathyavini
Copy link
Collaborator Author

kathyavini commented Dec 3, 2024

Ah please hold off on re-review, I think there is a bug!! 🐞🙏

Edit: Please nevermind me, it mainly impacts complete so I can fix it on the other PR anyway!

...but while I'm waiting on re-review I'll just go ahead and fix it here :)

@kathyavini kathyavini marked this pull request as draft December 3, 2024 21:11
@kathyavini kathyavini marked this pull request as ready for review December 3, 2024 21:15
purposeOptions is re-created on every render; it is 'purposes' that is the useQuery hook return
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