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

[Project1/조민철] Colors #6

Open
wants to merge 6 commits into
base: JSmini_datalater
Choose a base branch
from

Conversation

datalater
Copy link
Collaborator

참고: 프로젝트별로 브랜치를 생성하려고 Project1에 대한 PR을 새로 올립니다

rgba 랜덤값을 만드는 방식으로 구현했습니다.

Copy link
Member

@AlangGY AlangGY left a comment

Choose a reason for hiding this comment

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

안녕하세요 민철님!
항상 새로운분의 코드를 살펴보는것은 즐거운일인것같습니다.
내 코드를 살필때는 생각하지 않았던 부분들이, 이상하게 다른분들 코드를 보면 더 많은 생각을 하게 되더라구요. 이렇게 리뷰를 함으로서 되려 저 스스로 성장하고, 제 코드를 돌이켜보는 시간을 갖게 되는것같습니다.
민철님의 코드 덕분에도 그런 시간을 가질수 있었던것 같습니다. 감사합니다!
코드를 보며 들었던 생각들을 코멘트를 통해 남겨보았습니다!
수고하셨습니다 ㅎ

<meta charset="UTF-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Ramdon Color</title>
Copy link
Member

Choose a reason for hiding this comment

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

깨알같은 부분이지만.. title에 오타가 있군요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

랜돈 컬러 ㅋㅋㅋ 매의 눈 감사합니다~~ 변경해서 반영하겠습니다

Copy link
Collaborator Author

@datalater datalater Sep 14, 2021

Choose a reason for hiding this comment

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


const $body = document.querySelector('body');

const randomColorValue = () => Math.floor(Math.random() * 256);
Copy link
Member

Choose a reason for hiding this comment

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

제가 알기로는, Math.random()은 0이상 1미만의 난수를 생성하는것으로 알고있습니다.
그렇기 때문의 Math.random()*256으로는 256이라는 숫자는 나올수 없을거에요.
https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Math/random
해당 MDN 문서 참고해보셔도 좋을것같아요.

Copy link
Collaborator Author

@datalater datalater Sep 14, 2021

Choose a reason for hiding this comment

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

아, 이게 최대값이 255여서 난수 0~1미만으로 곱셈한 결과가 최소값 0부터 최대 255까지 나오도록 256으로 했습니다.

Copy link
Member

Choose a reason for hiding this comment

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

앗...! 그렇네요ㅋㅋ 잠시 최대값이 256이라고 생각했습니다.

<link rel="stylesheet" href="style.css" />
</head>
<body>
<button id="button">Click Me!</button>
Copy link
Member

@AlangGY AlangGY Sep 13, 2021

Choose a reason for hiding this comment

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

혹시 button에 class가 아닌 id 값을 넣으신 특별한 이유가 있을까요?
저는 정말 고유한 상황 ( 유일한 값 데이터) 이 아니라면, 이런 상황에서 class를 사용하는 편이라, id를 선택하신 이유가 궁금합니다.
해당 구현에는 button이 한개 뿐이기에 영향이 없겠지만,
id에는 조금 더 고유한 값을 넣는것이 좋다고 알고있어요. css에서 해당 id값을 선택자로 하여 다루고 계시는데, 이상황에서 만약 button이 여러개라면, button의 css속성을 일괄적으로 다루는데 어려움이 있지 않을까 싶습니다. 그렇기때문에 저는 css 재사용성 부분에 있어 class를 사용하는 것이 더 적합하지 않을까 생각했습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이번 프로젝트에서는 버튼이 하나인 게 명확해서 id 속성을 선택했습니다.

하지만 말씀하신 대로 재사용성을 고려하면 class 속성을 사용하는 게 더 적합하다는 것에 저도 동의합니다! 다음에도 같은 상황이 오면 class를 사용하는 게 확장 가능성 측면에서 더 낫다는 생각이 드네요ㅎㅎ

수정해서 반영하겠습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use class attribute instead of id attribute considering extensibility

Reveal the intention in class name of button
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