-
Notifications
You must be signed in to change notification settings - Fork 78
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
base: integration
Are you sure you want to change the base?
LF-4388 animal details sub components handle edit and readonly modes #3485
Conversation
Add ID LockedInput, mode prop, and update Storybook
Add breed filtering and clearing, and update stories
For now this is only non-interactivity; don't yet know if a separate disabled style is required
This is to keep the add Storybook stories resembling their initial appearance in app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 */} |
There was a problem hiding this comment.
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?
Description
This adds two more modes (
readonly
andedit
) to the subcomponent cards that display animal details (the original has been designatedadd
). This PR also:readonly
mode) to custom form elements that were missing them:Jira link: https://lite-farm.atlassian.net/browse/LF-4388
Type of change
How Has This Been Tested?
Checklist: