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

[himinji] 프론트엔드 1주차 미션 제출합니다. #7

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

Conversation

himinji
Copy link

@himinji himinji commented Nov 23, 2024

PR은 한 번 하면 다시 안 뜨나봐요 레포지토리를 삭제하고 다시 했다가 진땀 뺐어요 수동으로 하는 방법 찾아서 성공

Copy link
Collaborator

@hhhkdev hhhkdev left a comment

Choose a reason for hiding this comment

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

PR에 대해서 간단하게 안내해드리자면...

git은 코드 리뷰를 해보면서 알 수 있듯이 파일의 변경내역 기준으로 작동해요. PR 또한 파일의 변경 내역을 주입하고자 하는 것인데, PR을 한번 보내게 되면 더이상 새로운 변경 내역이 없는 상황이기에 PR이 안뜨는 것입니당

+) 한번 PR하면 그 이후로는 Push해도 어차피 같은 PR로 들어가는거 같더라구여

전반적으로 코드가 깔끔하고 좋네요..! 2주차 미션에서도 이번 미션처럼 열심히 해봅시당

Comment on lines +36 to +38
<p>ㅤ<img src="img2.jpg" alt="Loading..." width="30%">ㅤ<img src="img1.jpg" alt="Loading..." width="30%">ㅤ<img
src="img3.jpg" alt="Loading..." width="30%"> </p>

Copy link
Collaborator

Choose a reason for hiding this comment

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

alt 텍스트에 Loading...이라고 적힌게 의도하신건지는 모르겟지만 alt의 역할에 대해서 알아보시고 이용하면 더 좋을거 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

이미지를 많이 이용하신게 인상깊네여 이렇게 이미지를 나열할 때는 Flex라는 CSS의 속성에 대해서 알아보시면 더욱 편하게 나열할수도 있습니당


<p>&lt;<u>hi</u>minji.com&gt; <b>;</b> hi stands for <b>H</b>ong<b>i</b>k University.</p>

<hr style="margin-top: 2.5rem; ">
Copy link
Collaborator

Choose a reason for hiding this comment

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

따로 css 파일이 존재하고, 아래랑 중복되어 이용되고 있는 코드라 굳이 여기에서 스타일 작성 안하고 css 파일을 이용하는게 나은거 같네여

Comment on lines +1 to +6
const readline = require('readline');

const rl = readline.createInterface({
input: process.stdin,
output: process.stdout,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

인터페이스를 이용해서 코드를 짜셨군요..! 확실히 JS 내에서 바로 입력받고 출력하기 위해서는 인터페이스를 ㅣㅇ용하는 방식이 가장 대표적인가 봅니당

다만 다른 방식으로 하신 분들도 있으니 한번 살펴보시면 좋을거 같아용

Comment on lines +25 to +39
function calculate(input) {
const [operand1, operator, operand2] = input.split(' ');
const n1 = parseInt(operand1, 10);
const n2 = parseInt(operand2, 10);

if (operator === '+') {
return add(n1, n2);
} else if (operator === '-') {
return sub(n1, n2);
} else if (operator === '*') {
return mul(n1, n2);
} else if (operator === '/') {
return div(n1, n2);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

calculate 함수도 따로 구분해서 작성하시니 코드를 알아보기 편하네여

다만 입력받을 때 물론 올바른 값만 들어온다고 가정하긴 햇지만 input.split()한 리스트의 결과가 3개가 아닌 경우 구조할당에서 문제가 발생될 수도 있으니 분리해서 작성하는게 장기적으로 좋을거 같아용 -> 물론 이런 유지보수 고려하는 미션은 아니라 상관업습니당

Comment on lines +4 to +6
const handleClick = () => {
alert('Hello!');
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

딱 제가 의도한 대로 작성해주셧네요! onClick에 들어갈 함수를 따로 빼내어 작성하고, 이를 전달하는 방식의 코드 형식은 React에서 흔히 볼 수 있는 형식입니당

출력 또한 alert로 하신게 좋네용

return (
<div className="App">
<h1>React Project</h1>
<Button /> { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 { } 오타있습니당

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