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

feat UserCard #22

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat UserCard #22

wants to merge 3 commits into from

Conversation

mpate154
Copy link
Contributor

added userCard

@mpate154 mpate154 requested a review from miguel-merlin March 28, 2024 23:39
Copy link
Member

@miguel-merlin miguel-merlin left a comment

Choose a reason for hiding this comment

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

@mpate154 Solid PR, please make the changes I highlighted in the review. Keep it up!

src/components/UserCard.tsx Show resolved Hide resolved
src/components/UserCard.tsx Show resolved Hide resolved
src/components/UserCard.tsx Show resolved Hide resolved
import { useState } from "react"; //for the save button that hasnt' been implemented
import React from "react";
import sampleUserData from "../sample_data.json";
import { useDisclosure } from "@chakra-ui/react";
Copy link
Member

Choose a reason for hiding this comment

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

You are importing multiple times from @chakra-ui/react you can include all your @chakra-ui/react imports into a single one

@@ -1,8 +1,96 @@
function UserCard (): JSX.Element {
import { useState } from "react"; //for the save button that hasnt' been implemented
Copy link
Member

Choose a reason for hiding this comment

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

You are importing multiple times from react you can include all your react imports into a single one

Name:
<Editable>
{userName}
<EditablePreview /*need?*/ />
Copy link
Member

Choose a reason for hiding this comment

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

Yes this is needed


<ModalBody>
Name:
<Editable>
Copy link
Member

Choose a reason for hiding this comment

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

When the user enters input, you need to tell react to change the state. You have to include

<Editable defaultValue={name} onSubmit={(value) => setName(value)}>

Same for all other <Editable/>

@miguel-merlin
Copy link
Member

@mpate154 Do you have any questions on the comments I've made to your pull request?

@mpate154
Copy link
Contributor Author

mpate154 commented Apr 9, 2024 via email

@mpate154
Copy link
Contributor Author

mpate154 commented Apr 9, 2024 via email

@mpate154 mpate154 requested a review from miguel-merlin April 11, 2024 23:11
package.json Outdated
@@ -15,6 +15,7 @@
"@types/react-dom": "^18.2.18",
"axios": "^1.6.7",
"framer-motion": "^11.0.5",
"mongodb": "^6.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why did you install the mongodb driver?
You can undo by running

npm uninstall mongodb

<Route path='/' element={<HomePage />} />
<Route path='/dashboard' element={<DashboardPage />} />
<Route path='/blog' element={<BlogPage />} />
<Route path="/" element={<HomePage />} />
Copy link
Member

Choose a reason for hiding this comment

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

We usually want to keep the pull request atomic, meaning the PR only focuses on one change, usually styling changes like this have to be done in another PR

EditablePreview,
} from "@chakra-ui/react";

import { setTokenSourceMapRange } from "typescript";
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

<Modal isOpen={isOpen} onClose={onClose}>
<ModalOverlay />
<ModalContent>
<ModalHeader>{userName} Information</ModalHeader>
Copy link
Member

Choose a reason for hiding this comment

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

nit:

{userName}'s Information

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