[WIP] Upgrade conform version#2
Conversation
edmundhung
left a comment
edmundhung
left a comment
There was a problem hiding this comment.
I will double check all the changes tomorrow and ping you when it is ready :)
| email: z.string({ required_error: 'Email is required' }).email('Email is invalid'), | ||
| password: z.string({ required_error: 'Password is required' }), |
There was a problem hiding this comment.
Conform now strips empty string to undefined to utilize zod required validation and there is no need to set min(1) anymore. Should I apply the same change to the NoteEditorSchema?
// Instead of this
const NoteEditorSchema = z.object({
title: z.string().min(titleMinLength).max(titleMaxLength),
content: z.string().min(contentMinLength).max(contentMaxLength),
})
// Change it to this?
const NoteEditorSchema = z.object({
title: z.string().max(titleMaxLength),
content: z.string().max(contentMaxLength),
})| }, | ||
| "error": { | ||
| "content": "String must contain at least 1 character(s)" | ||
| "content": "Required" |
There was a problem hiding this comment.
Because Conform is utilizing the zod required check now.
|
|
||
| return ( | ||
| <fieldset ref={ref}> | ||
| <fieldset ref={ref} {...conform.fieldset(config)}> |
There was a problem hiding this comment.
Was it intentional to omit the aria attributes?
There was a problem hiding this comment.
Yeah, just because I didn't want it to distract from things. It's great that those are added automatically now so we don't need to worry about it. This change is perfect 👍
|
We might need to update the README on exercise 4 as well. Since Conform does type coercion now, there is no need to check if the file is empty. // Before
const schema = z.object({
profile: z
.instanceof(File)
// When browser constructs a form data from an empty file input, a default file
// entry would be created. we can validate this by checking the filename and size.
.refine(file => file.name !== '' && file.size !== 0, 'Profile is required'),
})
// Now
const schema = z.object({
profile: z.instanceof(File, { message: 'Profile is required' }),
}); |
kentcdodds
left a comment
kentcdodds
left a comment
There was a problem hiding this comment.
This looks awesome to me! Thank you so much for getting these changes implemented, published, and making this PR!
| email: z.string({ required_error: 'Email is required' }).email('Email is invalid'), | ||
| password: z.string({ required_error: 'Password is required' }), |
|
|
||
| return ( | ||
| <fieldset ref={ref}> | ||
| <fieldset ref={ref} {...conform.fieldset(config)}> |
There was a problem hiding this comment.
Yeah, just because I didn't want it to distract from things. It's great that those are added automatically now so we don't need to worry about it. This change is perfect 👍
That's awesome. Yes, let's make that update too 👍 Thanks! |
edmundhung
force-pushed
the
upgrade-conform
branch
2 times, most recently
from
9563042 to
ffa2e43
Compare
edmundhung
force-pushed
the
upgrade-conform
branch
from
ffa2e43 to
7ef165b
Compare
|
This is ready @kentcdodds! The changes are huge so it might be easier to check the commit one by one. Please be aware that the impact for removing the |
|
Also, I kept this message in the file as the new |
|
@edmundhung, thank you so much for doing this. It's an enormous help at a time where I'm really pressed for time getting all these workshops prepared. I really appreciate it!
The diff is so big that it's not auto-focusing that, I don't know what message you're talking about. But I'll review everything post-merge 👍 Thanks a ton! |
|
Happy to help anytime! 🤓
I meant this message. |
|
Ah, gotcha. Yeah, that's fine :) Thanks! |
2 participants
Resolve #1