-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix(DEV-13054): add create counterparts from payables and approval policies #576
Conversation
|
...s/sdk-react/src/components/payables/PayableDetails/PayableDetailsForm/PayableDetailsForm.tsx
Outdated
Show resolved
Hide resolved
...eact/src/components/receivables/InvoiceDetails/CreateReceivable/sections/CustomerSection.tsx
Outdated
Show resolved
Hide resolved
...es/InvoiceDetails/CreateReceivable/sections/components/CounterpartAutocompleteWithCreate.tsx
Outdated
Show resolved
Hide resolved
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.
Nice work, thanks. The code changes are well-structured, and the introduction of the CounterpartAutocompleteWithCreate component promotes reusability.
But I have some additions that could be addressed.
Stylistic ones are conveyed as separate comments.
And more logical ones, I will write them here:
– There's considerable duplication of autocomplete logic in AutocompleteCounterparts and CounterpartAutocompleteWithCreate. May be we can do something about it now, or we can create a task for addressing it later
– ApprovalPolicyForm useEffect that sets initial form values is getting a bit long and complex, especially with the conditional logic for different trigger types. Consider breaking this down into smaller, more focused useEffect hooks, or extracting some of the logic into helper functions to improve readability
...provalPolicyDetails/ApprovalPolicyForm/AutocompleteCounterparts/AutocompleteCounterparts.tsx
Outdated
Show resolved
Hide resolved
...provalPolicyDetails/ApprovalPolicyForm/AutocompleteCounterparts/AutocompleteCounterparts.tsx
Outdated
Show resolved
Hide resolved
...provalPolicyDetails/ApprovalPolicyForm/AutocompleteCounterparts/AutocompleteCounterparts.tsx
Outdated
Show resolved
Hide resolved
.../components/approvalPolicies/ApprovalPolicyDetails/ApprovalPolicyForm/ApprovalPolicyForm.tsx
Outdated
Show resolved
Hide resolved
...es/InvoiceDetails/CreateReceivable/sections/components/CounterpartAutocompleteWithCreate.tsx
Outdated
Show resolved
Hide resolved
...unterpartDetails/CounterpartForm/CounterpartOrganizationForm/CounterpartOrganizationForm.tsx
Outdated
Show resolved
Hide resolved
...es/InvoiceDetails/CreateReceivable/sections/components/CounterpartAutocompleteWithCreate.tsx
Outdated
Show resolved
Hide resolved
@costa-monite Thanks for the feedback! I really appreciate it. You were absolutely right about the duplication in AutocompleteCounterparts and CounterpartAutocompleteWithCreate—I’ve refactored it to make it more reusable. As for the useEffect in ApprovalPolicyForm, I agree it could be improved, but the logic is quite complex and wasn’t written by me. I’m a bit worried about breaking something, so I’d prefer to leave it as is for now. Maybe we can create a separate task for it later. Let me know what you think! 😊 |
… feat/DEV-13054-create-counterpart
@ayubova Thank you for the clarification and for your work! Regarding the useEffect logic, I agree that in this case, it’s best to leave it as is for now. Thanks for pointing that out |
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.
Thanks for the updates
No description provided.