-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add url validation #2016
Add url validation #2016
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
34719b1
to
e1f60fb
Compare
e1f60fb
to
b613bf5
Compare
b613bf5
to
74a3d28
Compare
apps/desktop/package.json
Outdated
@@ -154,6 +154,7 @@ | |||
}, | |||
"packageManager": "[email protected]", | |||
"dependencies": { | |||
"electron-updater": "6.3.9" | |||
"electron-updater": "6.3.9", | |||
"@hookform/resolvers": "^3.9.0" |
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 should go to the devDependencies
.string() | ||
.min(1, "Tzkt Explorer URL is required") | ||
.url("Enter a valid Tzkt Explorer URL"), | ||
buyTezUrl: z.string().url("Enter a valid Buy Tez URL").or(z.literal("")), |
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.
also .optional()
?
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.
it's not necessary since .or(z.literal(""))
does the trick
563a428
to
d2feb7e
Compare
d2feb7e
to
b3057dc
Compare
b3057dc
to
d22622e
Compare
a495972
to
20ad8b9
Compare
d22622e
to
e1aba72
Compare
rpcUrl: urlScheme("RPC"), | ||
tzktApiUrl: urlScheme("Tzkt API"), | ||
tzktExplorerUrl: urlScheme("Tzkt Explorer"), | ||
buyTezUrl: urlScheme("Buy Tez").or(z.literal("")), |
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.
.or(z.literal("").optional())
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.
.optional()
isn't required in this case since form defaultValues are always equals empty string, so z.literal("")
it's appropriate in this case
name: network | ||
? z.string().optional() | ||
: z | ||
.string() | ||
.min(1, "Name is required") | ||
.refine(name => !availableNetworks.find(n => n.name === name), { | ||
message: "Network with this name already exists", | ||
}), |
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.
why not to just have network name in a hidden field? the code will become much simpler
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.
a hidden field implies the creation of additional input and should also be described in the schema, so I don't see any advantage to it
|
||
const result = schema.safeParse(invalidInput); | ||
expect(result.success).toBe(false); | ||
if (!result.success) { |
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.
just curious how result.success can be not false here? 🤔
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.
typescript doesn't know about it
e1aba72
to
8d30485
Compare
8d30485
to
4954ce4
Compare
Proposed changes
Task link
Types of changes
Steps to reproduce
Screenshots
Add the screenshots of how the app used to look like and how it looks now
Checklist