Conversation
✅ Deploy Preview for project-idea-board ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Initial scaffolding for an idea-page “Contact” flow, including a UI modal and a Netlify Function endpoint, plus CMS/schema updates to support a designated primary contact.
Changes:
- Add
ContactModaland wire it into the idea post template to submit to/.netlify/functions/contact. - Add
primaryContactto Decap CMS config + Gatsby schema/query/frontmatter. - Add Netlify Functions dependency and ignore local Netlify state folder.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Adds @netlify/functions dependency for the new serverless handler. |
yarn.lock |
Locks new Netlify Functions dependency (+ types). |
netlify/functions/contact.mts |
Introduces the contact Netlify Function (currently echo-style response). |
src/components/ContactModal.tsx |
New modal UI that collects sender info and POSTs to the Netlify function. |
src/templates/idea-post.tsx |
Adds “Contact” button/modal to idea pages and queries authors + primaryContact. |
gatsby/schema/base.gql |
Extends Frontmatter with primaryContact. |
static/admin/config.yml |
Adds primaryContact relation field to the CMS idea editor. |
src/pages/ideas/dev-example.md |
Adds primaryContact (and reorganizes some frontmatter content) in an example idea. |
.gitignore |
Ignores local .netlify folder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (event.body === null) { | ||
| return { | ||
| statusCode: 400, | ||
| body: JSON.stringify("Payload required"), | ||
| }; | ||
| } | ||
|
|
||
| const requestBody = JSON.parse(event.body) as { | ||
| senderName: string; | ||
| senderEmail: string; | ||
| recipient: string; | ||
| message: string; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return { | ||
| statusCode: 200, | ||
| body: JSON.stringify(requestBody), | ||
| }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/templates/idea-post.tsx
Outdated
|
|
||
| import { ContactModal } from "../components/ContactModal"; | ||
| import IconText from "../components/IconText"; | ||
| import Layout from "../components/Layout"; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/components/ContactModal.tsx
Outdated
| fetch("/.netlify/functions/contact", { | ||
| method: "POST", | ||
| body: JSON.stringify({ | ||
| senderName: senderName, | ||
| senderEmail: senderEmail, | ||
| recipient: recipientLabel, | ||
| message: message, | ||
| }), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ShrimpCryptid
left a comment
There was a problem hiding this comment.
Left a bunch of questions, overall looks good! Re-request me when you want me to take another look :>
netlify/functions/contact.mts
Outdated
| }; | ||
| }; | ||
|
|
||
| export { handler }; No newline at end of file |
There was a problem hiding this comment.
Nit: Missing whitespace at EOF-- can Prettier handle these file types?
There was a problem hiding this comment.
Hmm, yeah maybe missing that extension
src/components/ContactModal.tsx
Outdated
| const recipientLabel = hasPrimaryContact | ||
| ? primaryContact | ||
| : hasAuthors | ||
| ? (authors?.filter(Boolean).join(", ") ?? "the authors") |
There was a problem hiding this comment.
Is the nullish coalescing operator necessary here? Shouldn't authors be defined?
There was a problem hiding this comment.
So in this repo there will be a hardening step that will ensure this is defined, but that will come in a subsequent PR.
Currently authors is inferred and not an explicit part of our schema. We will add a more structured type and resolver to the gatsby node APIs when we know what we need. We will likely query both a name and a contactId to use in getting an environment variable (email address stored as secret).
For now gatsby is just inferring that its (string | nuull) | null
src/components/ContactModal.tsx
Outdated
| ? primaryContact | ||
| : hasAuthors | ||
| ? (authors?.filter(Boolean).join(", ") ?? "the authors") | ||
| : "fake default email inbox"; |
There was a problem hiding this comment.
Add a TODO or something so this doesn't get left in the code in the future? 🤭
There was a problem hiding this comment.
Will do, but there is kind of a TODO on all of this code, since we don't have a design and we don't know the shape of the data/query yet.
src/components/ContactModal.tsx
Outdated
|
|
||
| const handleSubmit = async () => { | ||
| try { | ||
| const response = await fetch("/.netlify/functions/contact", { |
There was a problem hiding this comment.
Should these function paths be stored somewhere as constants? What validates that these exist?
There was a problem hiding this comment.
I'm going ahead and storing these now as constants which is good feedback, this PR was the first time we are defining and calling netlify functions.
By default in netlify the file name for the function defined in netlify/functions is the path.
A constant doesn't hurt, but I don't currently know how I plan to do more robust validation, especially given that not everyone has access to the netlify CLI for this repo, currently just me and Megan, so we don't have a good set up for testing and verifying across users and reviewers.
I think best thing will be to do it in CI/CD via the netlify back end but I have to look into it a bit.
| console.log("Failed to send message. Please try again later."); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error sending message:", error); |
There was a problem hiding this comment.
Should this be shown on the modal as error text or something?
There was a problem hiding this comment.
Probably something like that for real failure points down the line
| if (response.ok) { | ||
| console.log("Message sent successfully, response:", response); | ||
| } else { |
There was a problem hiding this comment.
Should the modal close itself on a success and show an error message on failure?
src/components/ContactModal.tsx
Outdated
| <p> | ||
| <strong>Idea:</strong> {title} | ||
| </p> | ||
| {authors && authors.length > 0 && ( |
src/components/ContactModal.tsx
Outdated
| {authors && authors.length > 0 && ( | ||
| <p> | ||
| <strong>Authors:</strong>{" "} | ||
| {authors.filter(Boolean).join(", ")} |
There was a problem hiding this comment.
Repeated filtering here, could do it once up above
src/templates/idea-post.tsx
Outdated
| // MessageOutlined, | ||
| // StarOutlined, |
There was a problem hiding this comment.
Remove? I feel like it's better just to reimport these if the related comment block is uncommented in the future rather than leave these hanging out.
|
@ShrimpCryptid thanks for review. I pushed some changed in response to your feedback and also deferred on a couple things since this is still somewhat of a draft to just start setting up the use of netlify functions. Note that we aren't merging to main! |
Problem
Advances #44
Closes #60 and #61
Solution
Building out the initial scaffolding for our email/contact system.
In terms of design I think this a placeholder, aiming for a functional proof of concept.
In this PR:
contactThis is a fairly harmless template that we can build on top of as we decide what email tooling we want to use and how we want to store/manage email addresses.
Testing this function locally with
netlify cliseems to work, I'm not sure if it will deployed in preview or only when we merge, watching this as it moves through PR will be a useful scan of that system.