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

MISSION2 / 곽민준 #11

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

Conversation

mlnwns
Copy link
Collaborator

@mlnwns mlnwns commented Mar 26, 2024

1. 구현 모습

2. 해결 과정

배포 링크

image

  • 공통적으로 사용 되는 버튼에 대한 컴포넌트를 만들어 재사용하였습니다.

  • useState를 활용해서 특정 조건 일 때 버튼이 활성화,비활성화 되도록 하였습니다.

  • 커피 이미지를 opacity를 낮춘채로 10개 생성한 후에 클릭 시 opacity가 1로 설정되도록 하였습니다.


3. To 리뷰어에게

  • rem을 사용해야하는데 px로 사용하는데 습관이 되어버려서 이번 코드에서도 px을 사용했는데, rem을 사용해야 한다는 점 인식하고 있습니다. rem의 경우 border-radius나 padding과 같은 px을 사용하는 모든 부분을 rem으로 대체하면 된다고 알고있으면 될까요?
스크린샷 2024-03-26 오후 2 12 36 스크린샷 2024-03-25 오후 1 35 28
  • 컴포넌트를 생성했는데, 첫 글자를 소문자로 작성해서 삭제하고 다시 컴포넌트를 생성하면 위의 이미지와 같은 오류가 자주 발생하는 것 같습니다. 파일명을 수정했는데도 열려있는 파일명이 소문자로 나오기도 하고, 경로를 못찾기도 하는데 이런 경우 어떤 방식으로 해결하시는지 궁금합니다. 아예 다른 이름으로 재생성 하는것이 좋을까요?
스크린샷 2024-03-25 오후 1 39 26
  • 미션 화면을 보고 파란색으로 버튼을 만들어봐야겠다고 생각했는데, 색상코드를 어떻게 넣어야 할 지 감이 잘 안잡혔습니다. 미션화면의 버튼을 관리자도구에서 보는 방법도 있고 테일윈드를 사용하는 방법도 있겠지만 머릿속으로 생각한 색상 코드를 어떤 방식으로 찾으시는지 궁금합니다. (피그마에서 색상을 미리 선택하고 색상코드를 보는 방법도 있겠네요)

  • App.jsx 코드가 길다고 생각하는데, 컴포넌트를 더 나누었어야 했다고 생각합니다. 어떤 기준으로 컴포넌트를 나누어야 할 지 궁금하고, useState 부분도 App.jsx 말고 보통 어디에 코드를 작성하는지 들어보고싶습니다. 소중한 시간 내어 읽어주셔서 감사합니다!


@mlnwns mlnwns added the mission 미션 입니다! label Mar 26, 2024
@mlnwns mlnwns requested a review from loopy-lim March 26, 2024 08:58
@mlnwns mlnwns self-assigned this Mar 26, 2024
Copy link
Collaborator

@loopy-lim loopy-lim left a comment

Choose a reason for hiding this comment

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

먼가 주저리주저리 많아졌네요 ㅋㅋㅋㅋㅋㅋㅋ
개발자로서 성공 한거 같아요!! 주어진 조건에 대해 완벽하게 동작을 하는 것이자나요! 다만... 조금 더 확장성을 신경 쓰면 어떨까 생각이 들어요... 저는 개인적으로 네이밍, state를 잘 다루면 추후에 다시 코드를 볼 때 더 쉽게 이해가 되더라구요!!

1. rem vs px

rem을 사용해야하는데 px로 사용하는데 습관이 되어버려서 이번 코드에서도 px을 사용했는데, rem을 사용해야 한다는 점 인식하고 있습니다. rem의 경우 border-radius나 padding과 같은 px을 사용하는 모든 부분을 rem으로 대체하면 된다고 알고있으면 될까요?

저는 정말 대부분의 요소를 rem으로 사용합니다. rem과 px의 차이가 무엇이라고 생각하나요? 그러면 어느때에 쓰면 좋을까요? 먼가 질문을 다시 질문 드리는 것 같아서 죄송하지만, 한번 찾아보고 적어주시면 더 이해하는데 좋을 것 같아요.

2. ts 오류

컴포넌트를 생성했는데, 첫 글자를 소문자로 작성해서 삭제하고 다시 컴포넌트를 생성하면 위의 이미지와 같은 오류가 자주 발생하는 것 같습니다. 파일명을 수정했는데도 열려있는 파일명이 소문자로 나오기도 하고, 경로를 못찾기도 하는데 이런 경우 어떤 방식으로 해결하시는지 궁금합니다. 아예 다른 이름으로 재생성 하는것이 좋을까요?

vscode는 javascript, typescript에 대한 지원이 훌륭해요. 다만 파일 점차 커지거나 오래 켜져 있다면 오류가 나오더라구요. 아래와 같은 상황이 나오는 이유는 tsserver의 오류이거나, git의 오류일 가능성이 매우 높아요. 저는 먼저 tsserver를 재시작 하거나, git의 cache를 날려볼 것 같습니다.

3. 색상 잡는 법?

미션 화면을 보고 파란색으로 버튼을 만들어봐야겠다고 생각했는데, 색상코드를 어떻게 넣어야 할 지 감이 잘 안잡혔습니다. 미션화면의 버튼을 관리자도구에서 보는 방법도 있고 테일윈드를 사용하는 방법도 있겠지만 머릿속으로 생각한 색상 코드를 어떤 방식으로 찾으시는지 궁금합니다. (피그마에서 색상을 미리 선택하고 색상코드를 보는 방법도 있겠네요)

저는 색상표를 머리속에 넣고 다니고 싶지만.... 그정도의 인재는 되지 않는거 같아요 ㅠㅠㅠ ㅋㅋㅋㅋㅋ 그래서 추천하는 방법은 vscode에서 저 작은 파란색 을 클릭하시는 것니다! (여기서 피그마처럼 쉽게 바꿀 수 있어요)

4. 컴포넌트 분리 기준

App.jsx 코드가 길다고 생각하는데, 컴포넌트를 더 나누었어야 했다고 생각합니다. 어떤 기준으로 컴포넌트를 나누어야 할 지 궁금하고, useState 부분도 App.jsx 말고 보통 어디에 코드를 작성하는지 들어보고싶습니다. 소중한 시간 내어 읽어주셔서 감사합니다!

컴포넌트 분리 기준이라... 험... Visual적인 부분도 있고, 상태적인 부분도 있는 것 같아요. 특히 내부적으로 같은 상태를 써야하는 경우라면 합치고, 하나의 독립적인 상태라고 생각하면 나누는 것 같아요. 예를 들면 어떤 게시판이 있다고 가정해보아요.

  1. 게시판 전체를 하나의 상태라고 볼 수 있어요.
  2. 게시판 안의 item하나를 하나의 상태로 볼 수 있어요.
  3. 그렇다면 페이지를 넘기는 부분은 게시판에 있어야 할까요? 따로 만들어야 할까요?
    이를 정하는 것 모두 그 때의 상황에 따라 나누어야할꺼에요. 하나의 컴포넌트가 너무 크다면 state를 다루기 힘들기 때문에 이를 나누고, 과하게 나누었다면 파편화가 많이 되어 있기 때문에 이해하기가 어려워 하나로 합쳐야 할 것이에요. 즉 어느것이 정답인 것은 없어요.
    다만 다양한 방법으로 시도하면서 어느것이 좋은지 한번 비교해보는 것이 좋을 것 같네요. 이 다음의 코드는 자신만의 기준으로 한번 분리를 해본뒤, 어떤 것이 좋았는지 공유하면 좋을 것 같기도 하네요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 App.css를 쓰는 곳을 발견하지 못했어요... 혹시 제가 찾지 못한 곳이 있나요?

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

Choose a reason for hiding this comment

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

시험 끝난 후에 시간 여유 있을 때 천천히 피드백 반영해서 리팩토링 하도록 하겠습니다!


function App() {
const [count, setCount] = useState(0)
const [couponNum, setCouponNum] = useState(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

무엇이 좋다 라고 이야기 하고 싶어서 하는거 아니에요!(수정해 달라도 아닙니다.)

저는 이 이름을 보고 한번 고민을 했어요. coupon이라는 것에 num이 붙어야 할까? 그정도로 Number라는 것을 주어야 했을까? 이름에 타입이 들어가는 것은 최대한 막는 것이 좋다고 생각해요. javascript도 vscode에서 intellisence를 이용하게 된다면 type을 대충은 이해할 수 있거든요!

저는 그래서 이 함수의 네이밍을 couponCount, setCouponCount라고 지으고 싶어요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 그 이름이 더 좋은거 같아요! 숫자라는 이름보다는 count한다는 내용이 들어가야 읽는 사람이 더 이해하기 쉬울 거 같네요

Copy link
Collaborator

Choose a reason for hiding this comment

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

아니면 혹시 다른 좋은 이름이 무엇이 있을까요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

couponQuantity, setCouponQuantity같은 이름도 있겠지만, 특성 상 couponCount가 가장 적합한 것 같습니다!


function App() {
const [count, setCount] = useState(0)
const [couponNum, setCouponNum] = useState(0);
const [isDisabled, setIsDisabled] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 컴포넌트 안에서는isDisabled가 될 수 있는 요소가 너무 많은거 같아요. DefaultButton 2개 모두 이 isDisabled에 영향이 가 있네요. 조금더 명확하게 이름을 짓는거도 좋을 것 같아요. 예를 들면 isFullCoupon의 이름을 가지는 것은 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞네요! isDisabled라는 이름은 너무 추상적인거 같아요 isFull의 의미는 쿠폰이 가득 차있는지 아닌지로 해석하면 될까요?

Copy link
Collaborator

@loopy-lim loopy-lim Apr 1, 2024

Choose a reason for hiding this comment

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

사실 아래의 댓글을 보시면 조금 더 이해할 것 같아요.

사실 제가 궁금한 것은, 데이터를 나눈 이유에요!
저는 개인적으로 저 3가지 데이터가 모두 한개라는 것으로 생각하기 때문이에요.

조금 더 그 이유를 설명하자면, 파생된 상태에 대해 찾아보면 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

파생된 상태에 대한 글을 찾아서 읽어보고 있는데, 파생된 상태가 어떤 느낌인지는 알겠으나 어떻게 사용해야 할 지에 대해서는 분명하게 와닿지가 않은거같아서 추가적으로 공부해보고 정리해보도록 하겠습니다!

Comment on lines +8 to +9
const initialOpacity = new Array(10).fill(0.4);
const [opacityList, setOpacityList] = useState(initialOpacity);
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 각 버튼에 대한 상태를 관리하고 싶었나요?
그럼 그 상태의 값을 opacity로 한 이유는 무엇인가요?
다른 방법으로 상태를 관리하면 그 차이가 무엇일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

상태의 값을 boolean값으로 false와 true로 나누고 true일 때 opacity를 1로 설정하는 방식으로 해도 괜찮겠네요 기존 방법은 opacity 상태의 값이 바뀜에 따라 무엇을 의미하는지 모른다면, boolean값으로 상태의 값을 지정하면 cofeeIcon이 채워진건지 아닌건지 한 눈에 파악하기가 좋을 것 같습니다!

Comment on lines +11 to +23
const handleOrderClick = () => {
const newCouponNum = couponNum + 1;
setCouponNum(newCouponNum);

const newOpacityList = opacityList.map((opacity, index) => {
return index < newCouponNum ? 1 : 0.4;
});
setOpacityList(newOpacityList);

if (newCouponNum >= 10) {
setIsDisabled(true);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

핸들링하는 함수로 빼는건 좋은거 같아요!

width: 100%;
height: 1px;
background-color: #e0e0e0;
margin: 30px 0 50px 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

같은거긴 한데... 아래와 같이 쓸 수 있긴 해요(그냥 참고용)

Suggested change
margin: 30px 0 50px 0;
margin: 30px 0 50px;

const CoffeeIcon = styled.img`
width: 80px;
height: 80px;
margin: 5px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

서로가 띄워진 경우에는 margin과 padding 둘 중 어느 것을 사용해도 괜찮아 보이나, 저는 개인적으로 padding을 더 추천합니다.
그 이유는.... button일 때 접근성 부터 시작해서... margin collapse까지 있는데 한번 찾아보시는 것을 추천드려요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

항상 margin을 사용하고 padding을 고려하지 않았었는데, margin collapse라는 문제점이 있군요! 직접 사용하면서 해당 현상을 겪어보진 않았는데, 추후에 해당 문제를 직접 겪어보도록 실험해봐야겠습니다!

background-color: #4888ff;
border-radius: 5px;
border: none;
color: white;
Copy link
Collaborator

Choose a reason for hiding this comment

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

white도 좋습니다. 다만 나중에 white가 white가 아니게 되는 경우가 있어서...(그냥 디자인상으로 말하는거에요!) 이를 어떻게 하면 좋을까요? 또한, 만약 darkmode가 된다면 어떻게 되어야할까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

14번째 줄 처럼 색상 코드를 입력하는걸까요?
darkmode일 때는 위처럼 코드를 작성하면, white색상이 그대로 반영되는데, @media (prefers-color-scheme: dark) 미디어 쿼리를 활용해서 다크모드일 때 다른 스타일을 적용할 수 있을 것 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

어.... 제가 원하던 것은 var()에요! css도 변수처럼 속성을 사용할 수 있다는 것을 알려드리고 싶었어요.
예를 들어 --x, --y같이 미리 var를 지정 한뒤, window에서 mouse의 위치를 --x, --y로 따라가게 설정하면 마우스에 따라 움직이는 div요소를 만들 수오 있죠.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CSS변수라는 것과 앞에 --를 붙인 것들이 무엇인지 모르고 있었어요! 자료 검색해서 자세하게 찾아보도록 해야겠습니다 감사합니다!

margin: 30px 10px 30px 10px;
font-weight: bold;
padding: 10px 20px;
cursor: pointer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

button의 기본적인 css 속성은 무엇이 있을까요? 아래에 적어주세요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

button {
display: inline-block;
padding: 6px 12px;
margin: 0;
font-size: 14px;
font-weight: normal;
line-height: 1.42857143;
text-align: center;
white-space: nowrap;
vertical-align: middle;
cursor: pointer;
-webkit-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
background-image: none;
border: 1px solid transparent;
border-radius: 4px;
}

21번 줄은 빼도 되겠네요!

Copy link
Collaborator

@loopy-lim loopy-lim Apr 1, 2024

Choose a reason for hiding this comment

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

tailwindlabs/tailwindcss#8961
하지만 유명한 라이브러리 중 하나인 tailwindcss에서는 default로 cursor가 없다고 하네요. 저는 있는 줄 알았는데 아니었네요. 저도 사실 방금 알았어요. 이래서 css reset을 하고 넣으라는건가...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

테일윈드 아니긴하지만, 코드 작성했을 때 cursor 관련 코드 있는 것과 없는 것의 차이가 분명히 있었어서 코드 넣었던거 같네요!

Comment on lines +23 to +25
&:hover {
background-color: #366fc9;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오오... sass를 사용하시는 군요!! 저는 아직도 이 sass가 어렵답니다...ㅠㅠ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mission 미션 입니다!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants