-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: new org unit form #421
Conversation
✅ Deploy Preview for dhis2-maintenance-app-beta ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 a lot for this, overall looks great!
Preliminary review :)
0bed3e1
to
8df351c
Compare
8df351c
to
1795788
Compare
… when there are no org units
ea4d691
to
bbb94fa
Compare
693420d
to
8bd6709
Compare
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.
Some minor notes.
Mostly related to minor cleanups, so nothing crucial. Thanks a lot for this!
<div className={classes.selectedOrgUnitBox}> | ||
<OrganisationUnitField | ||
name={'parent'} | ||
singleSelection |
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 could potentially add some logic to the OrganisationUnitField
to be aware of singleSelection
(now it just passes it through to the UnitTree - and then operate on reference
instead of an array
. Then we wouldn't have to change the type of the parent
in formValue
? However it does have the complication of "changing" the API, so maybe not worth?
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.
let's have a quick chat about this :)
code: z.string().trim().optional(), | ||
description: z.string().trim().optional(), | ||
image: z.object({ id: z.string() }).optional(), | ||
attributeValues: z |
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.
Can we use the helpers from modelFormSchemas
, eg. withAttributeValues
here?
|
||
const { withAttributeValues } = modelFormSchemas | ||
|
||
export const organisationUnitSchema = withAttributeValues.extend({ |
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.
Missing id
now. Could also us identifiable
from modelFormSchemas
.
} | ||
|
||
return ( | ||
<FieldRFF<string | undefined> name={fieldName}> |
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 don't need this. This will create a new listener
for the field - and we already have useField
above.
Edit: I updated this in the commit for image preview
caf07d1
to
dbd0df2
Compare
🎉 This PR is included in version 0.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.