-
Notifications
You must be signed in to change notification settings - Fork 0
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
[COD-53] #59
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/codingduckscyf/miricyl/GoTk9UnccEPns6FUfa3rdR4e2PzC |
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 a few questions Oksana. The one about the config file can be ignored really.
@@ -1,6 +1,7 @@ | |||
import sql from "~/lib/postgres"; | |||
|
|||
const handler = async (req, res) => { | |||
const issuesTest = await sql`SELECT * FROM issues;`; |
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.
I think this can probably be removed, I presume you were using this to log the output somewhere or something like that.
domains: [ | ||
"picsum.photos", | ||
"dummyimage.com", | ||
"images.unsplash.com", | ||
"insidethemagic-119e2.kxcdn.com", | ||
"saltinmycoffee.com", | ||
"breedingbusiness.com", | ||
"unsplash.com", | ||
], |
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.
My first thought on this is that I wonder if this can be done dynamically. Really not sure if that's possible so it'll be worth having a read of the documentation. My understanding is that these domains determine where an image can be hosted to be used in the next/Image
component so I'm just wondering what would happen if an admin user uploaded an image that was hosted somewhere different.
This might not even be a possibility, in which case you can ignore this whole comment.
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.
I thought about it too, but it seems like it's not possible yet. (or maybe I just missed something)
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.
check the comments here:
vercel/next.js#18311
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.
components/Form/index.js
Outdated
console.log(error.path); | ||
console.log(error.inner); |
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.
Do you definitely want to have both of these in? I'd guess you've been using them for troubleshooting.
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.
nope, will delete them now
components/Form/index.js
Outdated
const data = { | ||
title: titleRef.current.value, | ||
description: descriptionRef.current.value, | ||
content_type: contentTypeRef.current.value, | ||
img_url: imgUrlRef.current.value, | ||
video_url: videoUrlRef.current.value, | ||
relations: Number(issueIdRef.current.value), | ||
}; |
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.
I would have personally done this using useState
for each of these, but then each of them would also have required a click handler function. This is also just the way I tend to do things. This is a neat alternative.
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.
Kerim said the same thing, but I'm not sure I know how to do it with useState().
It would be great if you could help me with that
//data obj for the form | ||
const data = { | ||
title: "", | ||
description: "", | ||
content_type: "", | ||
img_url: "", | ||
video_url: "", | ||
relations: "", | ||
}; |
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 declare this here? Why not declare it inside the
component to save having to pass it in as a prop?I'm potentially missing something 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.
It's because I use the same form for updating and adding the content.
So I need to pass the data object as a prop to the form.
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.
Yes. Good work.
reusable form and add-content page added