Skip to content

React evaluation#27

Open
tomwinskell wants to merge 30 commits into
projectshft:mainfrom
tomwinskell:main
Open

React evaluation#27
tomwinskell wants to merge 30 commits into
projectshft:mainfrom
tomwinskell:main

Conversation

@tomwinskell
Copy link
Copy Markdown

Live site at https://contact-list-nextjs-liart.vercel.app/. May be easier for demonstration than loading on personal computer as it uses a Postgres database.

Readme explains how to run it locally if you prefer.

Did not use propTypes because I think that would have been a waste of type. The project is typed with TypeScript for type safety instead.

…vides contacts loaded from json as context for the child components. All contacts, individual contact pages function.
…te range of conditions. Form validation context moved to new contact page, form moved into ui component.
…tion with zod. Connected to neonDB. Created API for post new contact and get all contacts.
…ider used to load individual contacts. Modal state managed at child level using useImperativeHandle and ref.
const contacts = useContext(ContactsContext);
const { id } = useParams();

const c = contacts.find((c) => c.id == id);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

be more literal

Suggested change
const c = contacts.find((c) => c.id == id);
const selectedContact = contacts.find((contact) => contact.id == id);

return (
<>
<PageHeading heading="Edit Contact" />
<Form values={c} update={true}/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what is c? there you see why readability is important

async function getContacts(): Promise<void> {
axios
.get('/api/contacts')
.then((res) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

make the argument name more meaningful

Suggested change
.then((res) => {
.then((response/serverResponse) => {

const data = res.data.contacts;
return data.map((c: ContactData) => keysToTitleCase(c));
})
.then((data) => setContacts(data))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

data is too generic

children: React.ReactNode;
}

export function ToastLogic(props: Props, ref: React.Ref<ToastHandle>) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

missing return type, if you are going typescript, go the whole way

}
}

async function handleDelete() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why es5? stick to es6

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.

2 participants