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

Update contact page to form that sends email upon submission #203

Closed

Conversation

misterbastean
Copy link

Currently set to send emails to Mailtrap for testing, so need to update SMTP settings in /pages/api/contact.js as needed.
Also added 2 environmental variables: EMAIL_USERNAME and EMAIL_PASSWORD, again for SMTP.

@vercel
Copy link

vercel bot commented May 28, 2021

@misterbastean is attempting to deploy a commit to the Teacherfund Team on Vercel.

A member of the Team first needs to authorize it.

@joelwass
Copy link
Member

joelwass commented Jun 1, 2021

This is awesome! Thanks @misterbastean we'll review sometime this week :)

@misterbastean
Copy link
Author

misterbastean commented Jun 2, 2021 via email

Comment on lines 29 to 39
if (res.status === 200) {
setName('')
setEmail('')
setSubject('')
setMessage('')
} else {
}
}).catch((err) => {
// TODO: Add error handling
console.log('ERROR:', err)
})
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps if sending the message fails it should notify the user in some way so they aren't expecting that it worked behind the scenes. Can the UI be updated with some kind of failure message/toast ?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. Added Chakra Toast to notify user email was sent or if there is an error. Still needs better error handling, because it's just a catchall right now, but it works fine for now.

pages/api/contact.js Outdated Show resolved Hide resolved
pages/api/contact.js Outdated Show resolved Hide resolved
pages/api/contact.js Outdated Show resolved Hide resolved
@misterbastean
Copy link
Author

@joelwass Updated PR. I'm new to contributing to OS on Github, so not sure I submitted correctly or if you get notified automatically, etc.

@vercel
Copy link

vercel bot commented Jun 11, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/teacherfund/teacherfund/AY7EG6RkKPMTi2gZMkZD9EgLK4v5
✅ Preview: https://teacherfund-git-fork-misterbastean-update-contact-teacherfund1.vercel.app

@joelwass
Copy link
Member

@joelwass Updated PR. I'm new to contributing to OS on Github, so not sure I submitted correctly or if you get notified automatically, etc.

Yea the reviewers get emails when pushes are pushed :) thanks for the changes! looks good, i'm going to let it deploy to our preview branch so i can test it in the UI

})
}
}).catch((err) => {
// TODO: Add better error handling
Copy link
Member

Choose a reason for hiding this comment

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

can you remove these console logs? also - you set the description of the toast to err, should it be err.message?

Copy link
Member

@joelwass joelwass left a comment

Choose a reason for hiding this comment

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

For some reason our CI breaks on this PR, i'm digging into why then will merge after consoles removed :)

console.log('Error sending email:', err)
res.status(500).json({ success: false, err })
} else {
console.log(info)
Copy link
Member

Choose a reason for hiding this comment

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

remove consoles :)

@misterbastean
Copy link
Author

Consoles removed, fixed typo (err => err.message). Thanks for catching those!

@joelwass
Copy link
Member

Thank you! I'm going to wait to merge until i figure out our CI issue but i'll merge when we get that figured out, thanks again!

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.

None yet

3 participants