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 / 김동완 #15

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

Conversation

dxwwan
Copy link
Collaborator

@dxwwan dxwwan commented Mar 27, 2024

1. 구현 모습

2024-03-27.2.56.25.mov

배포링크
https://donut-study.vercel.app/mission2


2. 해결 과정

  • 이번 주차는 신입 모집TF로 인해 시간이 부족해 필요한 기능과 저번 주에 부족했던 점들을 조금 보완해보고자 했습니다!

  • SCSS를 처음으로 적용해 보았습니다.

    • 확실히 그냥 CSS를 사용하는 것보다 @mixin태그를 사용해 전처리하는 것이 편리했습니다.
    • 원래 CSS-in-JS를 자주 사용해서 공통 컴포넌트를 사용하는 방식을 지향했습니다.
    • CSS-in-CSS를 사용하니 공통 컴포넌트의 타입을 프롭으로 넘겨주는 편리한 방식을 사용할 수가 없었습니다.
    • 이에 대한 해결과정은 다음 미션에 좀더 고민해볼 계획이고 현재는 공통 컴포넌트에서 CSS전처리가 된 여러 컴포넌트를 선언해 주는 것으로 해결했습니다.
  • 컴포넌트의 의존성을 줄이기 위해 상태를 훅에서 관리했습니다.


3. To 리뷰어에게

  • SCSS를 좀 더 잘 활용하는 방법에 어떤 사례가 있을까요?

  • CSS-in-JS에 너무 익숙해진 나머지 프롭스로 스타일을 넘겨주는 방식이 너무 익숙해진 것 같습니다. 성능의 차이가 많이 나는 것은 알고 있습니다만 CSS-in-JS의 편리함과 타입의 안전성을 배제할만큼 CSS-in-CSS이 장점이 큰걸까요?

  • 상태에 관해서 많은 고민을 할 시간은 갖지 못했지만 어떤 부분을 개선해볼 수 있을까요?

  • 시간내주셔서 리뷰해주시는 모든 분들께 감사드립니다..! 좋은하루 되십쇼~~!!!


geongyu09 and others added 30 commits March 12, 2024 22:41
@dxwwan dxwwan added the mission 미션 입니다! label Mar 27, 2024
@dxwwan dxwwan requested a review from JUDONGHYEOK March 27, 2024 06:09
@dxwwan dxwwan self-assigned this Mar 27, 2024
Copy link
Collaborator

@JUDONGHYEOK JUDONGHYEOK left a comment

Choose a reason for hiding this comment

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

안녕하세요 동완님! 신입 모집TF 너무 수고많으셨어요~~ 늦은 시간까지 좋은 동아리원들을 선별하기 위해 노력해주셔서 감사합니다! 바쁘신 와중에도 이번 미션까지 수행하시느라 너무 고생많으셨어요! scss를 처음 사용하신 것 같은데 사용하시면서 느낀점까지 적어주셔서 너무 감사해요!

그런데 미션1의 커밋들이 같이 들어온 것 같은데요. 리뷰를 하는데 조금 시간이 더 소요되는 것 같아요. 미션1의 코드들을 계속 이어서 사용하고 싶으시다면 현재 레포에 따로 동완님만의 브랜치를 만들어서 계속 이어나가는 것은 어떨까요?

또 전체적으로 주석이나 활용하지 않는 코드들이 보이는데요. PR을 올리시기 전에 한번씩 점검해주시면 좋을 것 같아요!

몇가지 커멘트들을 남겼는데요. 이해가 안가시는 부분이 있으시다면 리코멘트 달아주시길 바랍니다!

질문사항

1️⃣ SCSS를 좀 더 잘 활용하는 방법에 어떤 사례가 있을까요?

  • 사례라고 말씀드리긴 조금 민망한 것 같아요. 아직 scss문법에 많이 익숙해지시진 않은 것 같아서 공식문서 다시 읽어 보시는 걸 추천드려요. scss를 활용하면 변수도 쉽게 선언할 수 있고 import export도 편리하고, 함수로도 스타일 적용이 가능하고 등등 여러가지 기능들이 있어요. 팁이라기엔 기능에 가깝고 공식문서에 모두 나와있는 것들이여서 문서를 읽어보시는게 가장 큰 도움이 될 것 같습니다.

2️⃣ CSS-in-JS에 너무 익숙해진 나머지 프롭스로 스타일을 넘겨주는 방식이 너무 익숙해진 것 같습니다. 성능의 차이가 많이 나는 것은 알고 있습니다만 CSS-in-JS의 편리함과 타입의 안전성을 배제할만큼 CSS-in-CSS이 장점이 큰걸까요?

  • 이 문제는 거꾸로도 생각해볼 수 있을 것 같아요. CSS-in-CSS가 런타임에서 성능적으로 우수한 것은 명확히 지표로 나타낼 수 있는 사실이잖아요. 하지만 말씀해주신 편리함이라는 것은 결국엔 익숙함에 대한 요소가 많이 차지하는 개인적인 기준이 아니였을까요? 프롭스로 넘기는 기능은 결국 자바스크립트 변수를 직접적으로 사용할 수 있다는 말씀이시죠? 편리함보다 성능의 중요성이 더 높은 가치를 만들어 내는 애플리케이션일 수도 있을 것도 같구요.(타입의 안정성은 어떤 이야기인지 명확히 이해를 못하겠습니다.)
  • 결국 CSS-in-JS나 CSS-in-CSS 모두 스타일링을 하기 위한 도구일 것 같아요. 도구를 선택하는 기준은 애플리케이션마다 다를 것이구요. 예를 들면 말씀해주신대로 CSS-in-JS에서 편리함을 성능의 기준보다 높게 평가하는 팀원들이 있고 성능이 그렇게 중요하지 않고 CSS-in-JS가 익숙한 팀원들이 빠르게 개발해야하는 프로젝트라면 CSS-in-JS를 선택할 것 같구요. 그게 아니라면 CSS-in-CSS를 선택할 것 같아요.
  • 두 도구의 장단점은 명확하고 그것이 크냐 작냐에 따라 어떤 도구를 선택하는 것은 상황에 따라 다를 것 같아요. 오히려 명확하게 이것을 할거야 라고 단정 짓는 것이 더 위험한 생각인 것 같습니다! 이번 미션에서 sass를 처음 사용해보시고 장단점을 느끼신 것은 매우 칭찬드리고 싶어요. 하지만 처음 사용해시는 것이다보니 익숙치않아 아무래도 기존에 사용하시던 css-in-js가 더 생각이 나셨을 것이라고 생각합니다. 앞으로 계속되는 미션에서 다양한 시도를 해보시고 그 때 또 다시 고민을 해본다면 더 넓은 생각을 하실 수 있을거라 생각합니다.

(*두개 차이점을 잘 드러내는 글이 있어서 추천해요! 사실 제 개인적인 생각도 아래 아티클과 비슷하긴 합니다. https://junghan92.medium.com/%EB%B2%88%EC%97%AD-%EC%9A%B0%EB%A6%AC%EA%B0%80-css-in-js%EC%99%80-%ED%97%A4%EC%96%B4%EC%A7%80%EB%8A%94-%EC%9D%B4%EC%9C%A0-a2e726d6ace6)

3️⃣ 상태에 관해서 많은 고민을 할 시간은 갖지 못했지만 어떤 부분을 개선해볼 수 있을까요?

  • 현재 코드에서는 상태가 하나밖에 없어서 상태 자체에는 현재 개선할 부분은 보이진 않는 것 같아요. 상태란 무엇인지 스스로 정의를 내려보시고 앞으로 더 큰 프로젝트에서 어떻게 이를 관리할지 고민해보면 좋을 것 같아요. 기타 개선사항들은 커맨트로 남겨두었습니다!

일주일동안 학교 수업과 미션 같이 진행하시느라 너무 고생많으셨어요. 새로운 시도들도 해주신 것 같아 좋았습니다! 남은 미션들도 화이팅 해서 같이 성장해나가보아요~~

Comment on lines +1 to +9
@mixin font-color {
&red {
color: rgb(255, 100, 100);
}

&blue {
color: rgb(100, 150, 255);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

얘네들이랑 아래 button-color는 쓰이는 데가 없는 것 같은데요? 그리고 저라면 같은색상이 반복되는 것 같아서 red나 bule에 관한 변수를 만들어 놓을 것 같아요. 파일은 분리하구요.

Comment on lines +11 to +15
@mixin flexCenter {
display: flex;
justify-content: center;
align-items: center;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 친구도 반복되는 것 같아요. layout.scss를 만들어서 따로 정의해서 가져다 쓰는 것이 좋지 않을까요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이 친구도 반복되는 것 같아요. layout.scss를 만들어서 따로 정의해서 가져다 쓰는 것이 좋지 않을까요??

좋은 의견인 것 같습니다..! 감사합니다.
항상 css-in-js를 사용해왔다보니 익숙하지 않은 부분이 많아서 좀 보기가 불편하실 수도 있다고 생각됩니다..! 좀더 고민 후 정리해보겠습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

어유 아닙니다. 이렇게 익숙한 것에서 벗어나 새로운 도전을 해보는 것도 대단하신 것 같아요!! scss import 하는 방법에 대한 링크도 남겨둘게요! 😄
https://sass-lang.com/documentation/at-rules/import/

background-color: rgb(100, 150, 255);
color: white;
border: none;
margin: 1rem 0 1rem 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

1rem 0 으로 줄일 수 있을 것 같아요!

return (
<>
<Header />
<nav></nav>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이친구랑 아래 footer는 요소가 없는 것 같은데 존재하는 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이친구랑 아래 footer는 요소가 없는 것 같은데 존재하는 이유가 있나요?

추후에 미션이 많아지면 전체적으로 페이지를 좀 꾸며보려고 일단 만들어놨습니다..!
컴포넌트 선작업은 좋지않은 습관일까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

애플리케이션에 큰 영향을 주진 않겠지만 개인적으로는 YAGNI에 해당하는 사항인 것 같습니다. 애플리케이션이 어떻게 발전해나갈지 모르는 상황에서 이렇게 될거야라고 추정하여서 개발하는 것은 프로그램의 복잡성을 높이고 버그의 발생확률을 높인다고 해요. 그리고 이정도 작업은 기능이 나왔을 때 구현이 되어도 충분한 것 같다고 생각해요!

const { coupon, isCouponFull, getCoupon, orderByCoupon } = useCoupon();
return (
<div className="coupon-event-container">
<TextLarge className="coupon-event-title">쿠폰 이벤트</TextLarge>
Copy link
Collaborator

Choose a reason for hiding this comment

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

className이 안들어가고 있을 것 같은데요. 이것 외에도 prop을 안뚫어줘서 component에 className이 다 안들어가고 있을 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

className이 안들어가고 있을 것 같은데요. 이것 외에도 prop을 안뚫어줘서 component에 className이 다 안들어가고 있을 것 같아요.

빌드, scss에 신경쓰다보니 사소한 부분들을 많이 놓치고 있네요..! 감사합니다.

Comment on lines +1 to +12
import { Header } from "./Header";

export const Layout = ({ children }) => {
return (
<>
<Header />
<nav></nav>
<main>{children}</main>
<footer></footer>
</>
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

LayOut 을 Layout으로 이름을 바꿔주신 걸로 보이는데요. 왜 diff에 잡힐지 고민해보시면 좋을 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LayOut 을 Layout으로 이름을 바꿔주신 걸로 보이는데요. 왜 diff에 잡힐지 고민해보시면 좋을 것 같습니다

깃허브가 대문자와 소문자를 못잡는 것으로 아는데..캐싱된 것들을 지우거나 이름을 바꿔야하는 걸로 알고 있습니다. 더 좋은 방법이 있거나 다른 문제 때문이라면 더 고민해보겠습니다..!

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

Choose a reason for hiding this comment

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

Mission1것들도 다 diff로 잡히네요 다른분들은 main을 베이스 브랜치로 따주신 것 같은데 mission1 브랜치를 베이스로 따신 이유가 있을까요? 이렇게 계속 미션하다보면 commit이 계속 중첩될 것 같은데요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

배포를 하기위해 포크를 땄는데 mission1을 메인에 머지할 생각을 하지 못하고 메인과 비교해서 쌓이게 되었습니다..!
다음 주차 부터는 메인에 머지 후 최신화 해서 진행할 예정입니다..! 신경쓰지 못했네요 감사합니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 메인에 머지하시는 것보다 오리진 레포에 동완님 브랜치를 따로 파셔서 그 곳에 계속 쌓아가는 형식이 좋을 것 같아요! 메인에 머지하시면 다른 리뷰이분들께도 영향이 가니깐요!!

Comment on lines +1 to +4
.card-component-container {
display: flex;
justify-content: center;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

미션1 때 만들어주신 것 같은데 이름을 바꿀 필요가 있어보여요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

미션1 때 만들어주신 것 같은데 이름을 바꿀 필요가 있어보여요

브랜치관리를 잘못해서 전체적으로 리뷰를 진행하시기 불편하셨을 것 같습니다..!
의견 감사합니다.

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

Choose a reason for hiding this comment

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

주석은 제거해주세요~

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.

여러 컴포넌트에 대한 스타일이 혼재되어 있어서 파일을 분리할 필요가 있어보여요

CSS-in-CSS를 처음 사용하다보니 css파일이 너무 많아지는게 불편하다고 생각해서 이렇게 진행했습니다..!
많은 스타일을 정의하지 않을 것 같아서 하나에서 관리해볼까 생각했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

오호 그럴수도 있겠군요. 다만 scss는 모듈화하기 아주 좋은 라이브러리라고 생각해요. scss 공홈에서도 module을 기본적인 기능으로 소개하고 있구요. 다음 스텝에서는 적용하시면 아주 편리함을 느끼실 것 같아요~

@Klomachenko Klomachenko changed the title MISSION2/김동완 MISSION2 / 김동완 Mar 28, 2024
};

const orderByCoupon = () => {
switch (true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 좋아요!! if나 switch는 호불호의 영역이 강하다고 생각해서 동완님께서 switch문이 더 잘읽힌다 생각되시면 유지해주셔도 좋을 것 같아요!

Comment on lines +1 to +4
.card-component-container {
display: flex;
justify-content: center;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

아유 아닙니다. 덕분에 이전 코드보면서 동완님의 스타일도 파악하고 좋았어요ㅎㅎ 다음에는 신경써주시면 다음리뷰어가 더 수월하게 리뷰할 수 있을 것 같아요!!

<App />
</React.StrictMode>,
)
ReactDOM.createRoot(document.getElementById("root")).render(<App />);
Copy link
Collaborator

Choose a reason for hiding this comment

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

오우 좋아요 면접 때도 의외로 단골 질문이거든요. 이야기 나누시고 정리해두시면 도움이 되실 것 같아요!

Comment on lines +23 to +30
case isCouponFull():
alert("쿠폰 10개를 모두 모으셨습니다. 커피를 드립니다.");
setCoupon(0);
break;
case isCouponOver():
alert("쿠폰 10개를 모두 모았습니다. 커피를 드립니다.");
setCoupon(coupon - 10);
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 제가 이해하기론 full에서나 over에서나 10개를 차감해주는 것은 똑같다고 느꼈거든요. 10에서 0을 만드는 것이나 10개를 초과한 숫자에서 10개를 차감하는 것이나 10개를 차감하는 것은 똑같잖아요? 이에 따른 문구가 달라서 혹시 이것 때문에 구분하신건가 했어요.

const orderByCoupon = () => {
switch (true) {
case isCouponFull():
alert("쿠폰 10개를 모두 모으셨습니다. 커피를 드립니다.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 구조적인 문제는 너무 어려운 것 같아요. 애플리케이션이 복잡해질수록 신경써야하지만 역설적으로 더 신경쓰기 힘든 영역이기도 하구요. 이런 미션들 하실 때 의식적으로 나눠보시는 연습하시면 큰 도움이 되실 것 같아요!!

Comment on lines +2 to +4
&red {
color: rgb(255, 100, 100);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 scss에서도 유사하게 할 수 있습니다. 보통 객체 대신 파일단위로 모듈화를 많이 하는 것 같은데요. 변수화나 적용하신 mixin을 잘 활용하시면 편리하게 스타일을 통일할 수 있습니다.
다만 이 코드는 mixin을 가진 element에 red 엘리먼트에 스타일을 조정하는 것이라서 문법상으로 올바르지 않은 코드에요

Copy link
Collaborator

Choose a reason for hiding this comment

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

오호 그럴수도 있겠군요. 다만 scss는 모듈화하기 아주 좋은 라이브러리라고 생각해요. scss 공홈에서도 module을 기본적인 기능으로 소개하고 있구요. 다음 스텝에서는 적용하시면 아주 편리함을 느끼실 것 같아요~

Comment on lines +3 to +13
export const TextSmall = ({ children }) => {
return <p className="text-small">{children}</p>;
};

export const TextMedium = ({ children }) => {
return <p className="text-medium">{children}</p>;
};

export const TextLarge = ({ children }) => {
return <p className="text-large">{children}</p>;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 공통코드들을 줄이고 스타일을 공통화 하는 아주 좋은 의도신 것 같아요. 조금만 더 다듬으면 훌륭해질 것 같습니다!

Comment on lines +1 to +3
export const getImgUrlBy = (imgName) => {
return `src/img/${imgName}`;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

이미지 링크에 넣어주는 것보다 이미지를 직접 import하는 것 같아요. 이것에 대한 이점은 동완님이 직접 알아보시면 좋을 것 같습니다!

Comment on lines +8 to +14
<Suspense
fallback={
<div>
<p>loading...</p>
</div>
}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://react-ko.dev/reference/react/Suspense#usage 여기에 note를 확인하시면 좋을 것 같아요. 지금 상태에서 아무리 많은 렌더링을 한다고 해도 fallback ui는 나타나지 않을 거에요. 그렇다고 현상황에서 lazy가 그렇게 효과적인 상황도 아니라고 생각하기 때문에 도입 자체에 고민이 필요해보여요.

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.

3 participants