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

assignment: 한슬희 #1

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

Conversation

hanseulhee
Copy link
Member

@hyesungoh hyesungoh self-requested a review June 8, 2022 05:05
Copy link
Contributor

@hyesungoh hyesungoh left a comment

Choose a reason for hiding this comment

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

고생하셨어요 ~! 이쁘게 잘 해주셨습니다 👍 👍

궁금한 점이랑 추천 사항 몇개만 리뷰 남겨봤어요 ~

@@ -26,7 +26,7 @@
-->
<title>React App</title>
</head>
<body>
<body style="margin: 0">
Copy link
Contributor

Choose a reason for hiding this comment

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

과제라서 이렇게 하셨겠지만, 이런 경우 emotion global style로 처리하시는게 좋을 거 가타용

import Nav from "./components/Nav";
import profile from "./image/profile.jpg";
import Comment from "./components/Comment";

function App() {
return (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

시맨틱 태그에 대해서 공부해보고 적용해보시면 좋을 거 같아용 👍

}

function Comment() {
const [comment, setComment] = useState<IComment[]>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

댓글들이 저장되어 있는 상태라, 이름을 복수형으로 짓는 것이 좋지 않을까요?

<button css={buttonWrapper}>확인</button>
</form>
<div css={commentSort}>
{comment.map((todo: IComment) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{comment.map((todo: IComment) => (
{comment.map((todo) => (

여기서는 type을 따로 지정해주지 않으셔도 타입 추론이 되는데, 따로 지정해주신 이유가 있으실까요??

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 댓글들이 리스트 렌더링되는 것인데, todo라는 이름은 부적절한 거 같아요

@hyesungoh hyesungoh requested a review from NohWookJin June 16, 2022 08:28
Comment on lines +51 to +54
const text = css`
font-size: 1.1rem;
font-weight: 700;
`;
Copy link
Member

@NohWookJin NohWookJin Jun 19, 2022

Choose a reason for hiding this comment

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

emotion 사용이 나중에 관리할 때 더 쉬울 것 같다는 생각이 들었어요.
다음 번에 저도 꼭 사용해보겠습니다..😳

justify-content: center;
align-items: center;
border-radius: 4px;
width: fit-content;
Copy link
Member

Choose a reason for hiding this comment

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

fit-content로 width 속성값을 설정하는 것을 아직 한 번도 사용을 해보지 않았는데,
슬희님 만드신 거 보면서 저도 꼭 사용을 해봐야겠다는 생각이 들었습니다!

Comment on lines +21 to +23
<Inform title="🎁 Date of birth" summary="2001.03.26" />
<Inform title="✍🏻 Student ID" summary="202014135" />
<Inform title="✉️ E-mail" summary="[email protected]" />
Copy link
Member

Choose a reason for hiding this comment

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

이모티콘 활용을 정말 잘 하시는 것 같아요!
그리고 Inform 컴포넌트에서 가져와서 사용하시는 모습을 보고 깔끔하게 코딩을 잘 하시는 것 같아요..!

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.

3 participants