Skip to content

Conversation

goumi1009
Copy link
Collaborator

1. Colors

colors_1

2. Hex-Colors-Gradient

ezgif com-gif-maker (1)

3. Quote Generator

ezgif com-gif-maker

Colors/script.js Outdated

button.addEventListener('click', e => {
const containerElement = e.target.closest('.container')
containerElement.style.backgroundColor = randomColor(colors)
Copy link
Member

@gyulhana gyulhana Sep 15, 2021

Choose a reason for hiding this comment

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

document.body.style.backgroundColor = randomColor(colors)

라고 쓸 수도 있었을 것 같은데 container라는 클래스를 가진 div를 따로 생성하고 closest를 통해 요소를 선택하신 이유가 있는지 궁금합니다!

Copy link
Collaborator Author

@goumi1009 goumi1009 Sep 15, 2021

Choose a reason for hiding this comment

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

아 그러네요... 퍼블리셔하면서 생긴 습관 같아요. body에 직접 스타일을 주게 되면 후에 변경이나 추가 해야 하는 상황에서 난감한 일을 몇번 격었더니 body는 그냥 페이지가 그려지는 공간이라고 생각하고 그냥 두게 되더라고요.
하지만 지금 같은 상황에서는 그냥 body에 style 입히는 것도 좋았겠네요! 좋은 질문 감사합니다!
덕분에 생각없이 쓰는 코드에 대해 생각해보게 되었습니다! 😁👍

@gyulhana
Copy link
Member

gyulhana commented Sep 15, 2021

안녕하세요, 경미님. 코드리뷰를 맡은 규란입니다!
우선 경미님 코드 잘 봤고 3번까지 다 완성한 것 같으신데 코드는 Colors만 올라와있어서 우선 해당 부분만 살펴보았습니다 : )
colors라는 배열에 미리 색상 데이터를 넣어두고 랜덤으로 인덱스를 뽑아서 배경 색상을 바꾸는 방식으로 코드를 짜주신 거 같은데 코드가 전반적으로 엄청 깔끔하다는 인상을 받았습니다! 저도 본받아야겠다고 느끼구 갑니다..🐾

개인적으로 조금 아쉬웠던 지점은 버튼을 눌렀을 때 배경색으로 나올 수 있는 색상이 배열 안에 들어있는 색상으로 한정적이다 보니 RGB 코드나 Hex Color Code를 무작위로 뽑아주는 함수로 구현해보셨어도 좋았겠다 싶은 생각이 들었습니다..!

코드를 보다가 궁금했던 점은 해당 코드에 코멘트로 남겨두었습니다!
2~3번 프로젝트도 올려주시면 총총 보러와보겠습니다!
벌써 수요일인데 남은 이틀도 잘 보내시구 연휴 즐겁게 보내세요🔥

@goumi1009
Copy link
Collaborator Author

@gyulhana 안녕하세요 규란님 엉망인 PR 리뷰해주셔서 정말 감사합니다.
과제 끝내고 당연히 push했을거라고 생각했는데... 아니였네요. 😂
PR을 먼저하고 나중에 push해서 그런줄 알고 한참 엉뚱한걸 알아보고 있다 깨달았습니다.
시작전 이것저것 욕심냈지만 결국 시간에 쫓겨 예시 사이트를 기능을 옮기는 것에만 급급했던거 같습니다.
말씀해 주신대로 다음엔 좀 더 고민해서 구현해 보도록 하겠습니다.
다시 한번 리뷰 감사드립니다. 규란님도 즐거운 추석 방학 보내세요 😁👍

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