-
Notifications
You must be signed in to change notification settings - Fork 12
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
Sepehr/step five #34
base: main
Are you sure you want to change the base?
Sepehr/step five #34
Conversation
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.
Very nice!! Just a few comments on how to make things better, but don't worry about fixing them, keep up the good work
@@ -0,0 +1,43 @@ | |||
import { Button, Grid, TextField } from "@mui/material"; | |||
import { useState } from "react"; | |||
import NewTaskMutation from "./schema/mutations/NewTaskMutation"; |
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.
There's probably no need for a schema/
subfolder here, this makes it just confusing with what we call "schema" when we talk about databases
src: "./src", | ||
schema: "../schema/schema.graphql", | ||
language: "typescript", | ||
exclude: ["**/node_modules/**", "**/__mocks__/**", "**/__generated__/**"], |
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.
You can make sure all your relay compiled queries end up in the same folder by adding this:
module.exports = {
...
artifactDirectory: "./__generated__",
...
}
import { useState } from "react"; | ||
import NewTaskMutation from "./schema/mutations/NewTaskMutation"; | ||
|
||
export default function AddTask(props) { |
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.
we try to type our functional components:
export default function AddTask(props) { | |
interface Props { | |
... | |
} | |
const AddTask: React.FC<Props> = (props) => { ... } | |
export default AddTask; |
variant="outlined" | ||
size="small" | ||
value={newTask} | ||
onChange={handleChange} |
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.
No need for an onChange
handler here, if you're using the form submit event
const handleFormSubmit = (event: React.FormEvent<HTMLFormElement>) => { | ||
event.preventDefault(); | ||
if (newTask) { | ||
const trimmedText = newTask.trim(); |
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.
event.target.value.elements["<your-textfield-id>"]
should allow you to retrieve the value of the input on submit, this way you don't have to make your component stateful
const commit = (rowId: number, connectionId: string) => { | ||
const variables = { connections: [connectionId], input: { rowId } }; | ||
|
||
return commitMutation(RelayEnvironment, { |
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.
As a team standard, we use the mutation hook pattern provided by useMutation(...)
.
An example here: https://relay.dev/docs/api-reference/use-mutation/
And in our code:
here and here
This gives us a very "react" way of tracking whether the mutation is in flight in a component, and to avoid adding more states to components.
But the way you're doing it here works just great :)
No description provided.