-
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 4156 create animal batch creation form card #3201
Lf 4156 create animal batch creation form card #3201
Conversation
…s of count and sex details inputs
…okFormRegister type in Checkbox
|
||
export default function NumberInput({ | ||
initialValue, | ||
export default function NumberInput<T extends FieldValues>({ |
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 was getting a type error for the control prop so added this generic to prevent it.
const { inputProps, reset, numericValue, increment, decrement } = useNumberInput({ | ||
initialValue, | ||
initialValue: defaultValue, |
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.
The initial value type was giving an error so changed it use defaultValue prop that comes with react hook form
/> | ||
) : ( | ||
// @ts-ignore | ||
<Input |
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.
This Input
is handling text inputs, number inputs, date pickers, and passwords inputs all in one. It might be good to extract those out into different components and also use typescript to avoid ts-ignore.
Also the placeholder text appears to be darker than the one in react select.
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 added some minor comments, but it looks great! Thank you!
packages/webapp/src/components/Animals/AddAnimalsFormCard/AddAnimalsFormCard.tsx
Show resolved
Hide resolved
packages/webapp/src/components/Animals/AddAnimalsFormCard/AddAnimalsFormCard.tsx
Outdated
Show resolved
Hide resolved
packages/webapp/src/components/Animals/AddAnimalsFormCard/AddAnimalsFormCard.tsx
Outdated
Show resolved
Hide resolved
packages/webapp/src/components/Animals/AddAnimalsFormCard/styles.module.scss
Show resolved
Hide resolved
packages/webapp/src/components/Animals/AddAnimalsFormCard/styles.module.scss
Show resolved
Hide resolved
packages/webapp/src/components/Animals/AddAnimalsFormCard/styles.module.scss
Outdated
Show resolved
Hide resolved
@SayakaOno Thanks for your review - I've made the requested changes. |
import InputBaseLabel, { InputBaseLabelProps } from '../InputBase/InputBaseLabel'; | ||
import { useTranslation } from 'react-i18next'; | ||
import { ClearIndicator, MultiValueRemove } from './components'; | ||
import scss from './styles.module.scss'; | ||
import { MenuOpenDropdownIndicator } from './components'; |
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.
Tiny thing: This could be combined with the other import above
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 am really impressed by this one, I learned a lot haha
Everything looks pretty great, there are a few new things here I will still have to learn but happy to approve. I just had one question before doing so:
My only thought was that maybe since we are changing the location of our stories that we could maybe formalize it with a separate PR. I think I missed the discussion and I am curious about the pros and cons and was thinking since it could be a simple drag and drop to move the other stories we might be able to crush out the changed location for other stories so we don't support 2 systems?
@Duncan-Brain Thank you for reviewing The idea for having the stories in the same folder as the component was for better organization (atleast in my opinion and others seemed to agree). It's easier to know where a story is and usually when working on a frontend ticket you need both the component and story files and makes it easier to work on them when they're located together. It's also one less thing to have to think about when creating a new component - the story just goes in the same place as the component. I think a separate PR is a good idea. Might be frustrating manually moving every single story file though. |
@navDhammu I can get on board for that change, I think it lines up well with Anto's PR #3139 She actually put them in their own folder inside the component folder which maybe we should do also? The main con that I was thinking about was for excluding stories from the server and build process. We don't currently do it but we would need to parse out the files should we decide to streamline the server at some point vs. exclude 1 folder. Very doable but I think why many choose the existing format. The all files thing isn't necessary... but I figured if it takes like 1 hr it would be worth it so that we don't have two systems. We have a lot of transitioning architecture changes that might be hard to keep up with or confusing too haha |
@Duncan-Brain I like the structure in Anto's PR for the component library, and I think it's fine to use the stories folder like she did. Although I feel it's better to keep the folder structure flat, especially since most components would have one story file, so having just one file inside the stories folder seems a bit overkill to me. Regarding the server and build process - I'm not very familiar with how vite works but isn't storybook automatically not included in the build and dev server? There's a separate dev server for storybook and for the application code so I don't think one is included in the other, but I could be wrong. And I would prefer to just move all the story files in one PR for consistency. If creating a separate PR is decided then should I go ahead and change the stories for this PR back to the old structure, and create a new ticket for the change? |
…e inside src" This reverts commit 6962ac2.
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.
cool!
Description
CardV2
based on current designs, uses theas
prop to render custom html tag other thandiv
if needed. Example,<Card as='form'>
AnimalTypeSelect
andAnimalBreedSelect
components that can be used in other places.@SayakaOno There were some minor changes to NumberInput here, for example I added a
className
prop to allow changing styles for the outerdiv
that wraps the input field. This might create conflicts with your work in the soil amendment components.Jira link: https://lite-farm.atlassian.net/browse/LF-4156
Type of change
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
Checklist: