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-4388 animal details sub components handle edit and readonly modes #3485

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

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Oct 2, 2024

Description

This adds two more modes (readonly and edit) to the subcomponent cards that display animal details (the original has been designated add). This PR also:

  • Adds disabled states (for readonly mode) to custom form elements that were missing them:
    • SexDetails
    • ImagePicker
    • Note: still waiting to hear back from Loïc whether there is a visually distinct form of the upload button itself (when the user has not yet added an animal image), as there is not one on Figma.
  • Migrates the type and breed selects of the General Card to the dedicated components that were used in AddAnimalsFormCard (AnimalTypeSelect/AnimalBreedSelect)
  • Adds breed-clearing on type change functionality to the General Card, and migrates that behaviour from a useEffect to an onChange in both the General Card and the Basics Card (= AddAnimalsFormCard), as the onChange is a cleaner solution
  • Updates the Storybook stories + adds mock data to represent a selected animal:

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

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 kathyavini added the enhancement New feature or request label Oct 2, 2024
@kathyavini kathyavini self-assigned this Oct 2, 2024
@kathyavini kathyavini requested review from a team as code owners October 2, 2024 00:20
@kathyavini kathyavini requested review from antsgar and SayakaOno and removed request for a team October 2, 2024 00:20
@kathyavini kathyavini marked this pull request as draft October 2, 2024 16:01
@kathyavini kathyavini marked this pull request as ready for review October 2, 2024 17:17
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.

Looks great!!

Just a small thing I noticed: the sex details input gets highlighted when clicked, even though it's disabled.
Screenshot 2024-10-03 at 11 26 02 AM

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.

Couple of minor comments but code's looking great! 🔥 🔥

@@ -111,6 +101,7 @@ export default function AddAnimalsFormCard({
onTypeChange={(option) => {
trigger(`${namePrefix}${BasicsFields.TYPE}`);
onTypeChange?.(option);
breedSelectRef?.current?.clearValue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We possibly don't need a ref here at all,

resetField(`${namePrefix}${BasicsFields.BREED}`, { defaultValue: null });

seems to do the trick.
Also, not related to this PR, but do we need a ref for the UUID value? Couldn't we set it in the useEffect hook without storing it in a ref?

@@ -42,6 +43,7 @@ type CommonProps = {
label?: string;
optional?: boolean;
defaultUrl?: string;
disabled?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call this isDisabled to make it consistent with the other changes?

<div className={clsx(styles.sectionWrapper, mode === 'edit' && styles.edit)}>
{(mode === 'readonly' || mode === 'edit') && (
<>
{/* @ts-ignore */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not getting any errors when removing this, do we need it?

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