Skip to content
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

NW6 | Rabia Avci | Full-Stack-Project | Week-2 | New video form #26

Merged
merged 5 commits into from
May 18, 2024

Conversation

RbAvci
Copy link
Owner

@RbAvci RbAvci commented May 16, 2024

Completed these tasks for this ticket:

  • Added a user-friendly form for capturing new video recommendations.
  • The form supports fields for video titles and YouTube URLs.
  • Fetch and save to database is done.
  • Add CSS for new form component

Copy link

netlify bot commented May 16, 2024

Deploy Preview for watch-next-cyf ready!

Name Link
🔨 Latest commit 046903f
🔍 Latest deploy log https://app.netlify.com/sites/watch-next-cyf/deploys/6648bdb965311e00083bf89a
😎 Deploy Preview https://deploy-preview-26--watch-next-cyf.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@Ara225 Ara225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, nothing to complain about. Good work :)

background-color: #007bff;
color: #fff;
font-size: 16px;
cursor: pointer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason you're setting the cursor to pointer here? It should be pointer by default here https://developer.mozilla.org/en-US/docs/Web/CSS/cursor

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When its default it keeps an arrow, i wanted to make it a hand icon when hovering over the element :)

}

button:hover {
background-color: #0056b3;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always nice to have a hover style for buttons.

Overall, I really like what you're doing here, good use of flex and nice touches like border-radius and padding in inputs throughtout

"Content-Type": "application/json",
},
body: JSON.stringify({
title: e.target.title.value,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a perfectly acceptable way of handling inputs, especially for a small form, however it's worth being aware that there's a different (not necessarily better, it just serves a different purpose) way of doing this that you may need someday

https://react.dev/reference/react-dom/components/input#controlling-an-input-with-a-state-variable

@zelihapala zelihapala self-requested a review May 17, 2024 13:57
@RbAvci RbAvci merged commit 8743e0c into main May 18, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants