-
Notifications
You must be signed in to change notification settings - Fork 93
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
Feature/725 admin recurring donation person select #728
Feature/725 admin recurring donation person select #728
Conversation
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.
Thank you for the contribution @dimitur2204 🙌
It really rocks the boat, full of reusable code and thoughtful dev experience. ⛵
Added some minor points of discussion
src/common/hooks/confirm.ts
Outdated
closeHandler: () => void | ||
} | ||
|
||
const useConfirm = ({ onConfirm, onClose }: ConfirmProps): ConfirmHookProps => { |
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.
Awesome!
Let's rename the filename to match the hook name - useConfirm.ts
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.
Done in 95fedb9
showId: boolean | ||
} | ||
export default function PersonAutocomplete({ | ||
onSelect, | ||
showId, |
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.
Maybe we can add a default value to the boolean field showId
so we don't have to add it as showId={false}
showId: boolean | |
} | |
export default function PersonAutocomplete({ | |
onSelect, | |
showId, | |
showId?: boolean | |
} | |
export default function PersonAutocomplete({ | |
onSelect, | |
showId = false, |
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.
Done in 95fedb9
src/components/person/PersonInfo.tsx
Outdated
</Typography> | ||
<Box className={classes.infoWrapper}> | ||
<Typography> | ||
{t('person:info.createdAt')}: {format(parseISO(person.createdAt), 'PPpp')} |
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 are couple of pre-existing formatting functions at https://github.com/podkrepi-bg/frontend/blob/master/src/common/util/date.ts
Let's reuse one of them if matches the expected format or add this format to the utils so it can be reused later on.
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.
Done in 95fedb9
🎉 |
Motivation and context
Improve #708
Made the selection of a user for the recurring donation more user friendly and is now not a text field.
Added a
useConfirm
hook which according to the discussion here #697.Added a
<FormFieldButton />
component which mimics the look of a material field, but acts as a button.Added a
<PersonSelectDialog />
component which renders a dialog with an autocomplete where you can choose a person and get his information shown so that you are sure what you are selecting.Notes for the reviewer:
The API call for the
personList
would be a very heavy one and I don't think it's worth it just to populate anAutocomplete
. It would be a better idea to only fetch like a list of the{id, firstName, lastName}
or paginate that call in some way.However this is an admin platform and maybe it's not that big of a deal. I am not sure :|
Screenshots: