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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3,794 changes: 3,794 additions & 0 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
},
"dependencies": {
"react": "^18.2.0",
"react-dom": "^18.2.0"
"react-dom": "^18.2.0",
"styled-components": "^6.1.8"
},
"devDependencies": {
"@types/react": "^18.2.64",
Expand Down
Binary file added public/CoffeeIcon.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
42 changes: 0 additions & 42 deletions src/App.css
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.

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

Original file line number Diff line number Diff line change
@@ -1,42 +0,0 @@
#root {
max-width: 1280px;
margin: 0 auto;
padding: 2rem;
text-align: center;
}

.logo {
height: 6em;
padding: 1.5em;
will-change: filter;
transition: filter 300ms;
}
.logo:hover {
filter: drop-shadow(0 0 2em #646cffaa);
}
.logo.react:hover {
filter: drop-shadow(0 0 2em #61dafbaa);
}

@keyframes logo-spin {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}

@media (prefers-reduced-motion: no-preference) {
a:nth-of-type(2) .logo {
animation: logo-spin infinite 20s linear;
}
}

.card {
padding: 2em;
}

.read-the-docs {
color: #888;
}
108 changes: 80 additions & 28 deletions src/App.jsx
Original file line number Diff line number Diff line change
@@ -1,35 +1,87 @@
import { useState } from 'react'
import reactLogo from './assets/react.svg'
import viteLogo from '/vite.svg'
import './App.css'
import { useState } from "react";
import styled from "styled-components";
import DefaultButton from "./components/DefaultButton.jsx";

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가 가장 적합한 것 같습니다!

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.

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

const initialOpacity = new Array(10).fill(0.4);
const [opacityList, setOpacityList] = useState(initialOpacity);
Comment on lines +8 to +9
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이 채워진건지 아닌건지 한 눈에 파악하기가 좋을 것 같습니다!


const handleOrderClick = () => {
const newCouponNum = couponNum + 1;
setCouponNum(newCouponNum);
Comment on lines +12 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

react에서 useState의 state와 setState는 동기적으로 동작하는 것 같으나 내부적으로 상당히 복잡하게? 되어 있어요.
그렇기 때문에 이전 상태를 쓰고 싶다면 다음과 같은 방식을 이용하는 것을 추천드려요! (그 이유는 함 찾아보는 것도 좋을지도...?)


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

if (newCouponNum >= 10) {
setIsDisabled(true);
}
};
Comment on lines +11 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


const handleSubmitCoupon = () => {
alert("커피 무료 쿠폰이 발급되었습니다! 🎉");
setCouponNum(0);
setOpacityList(initialOpacity);
setIsDisabled(false);
};
Comment on lines +25 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

한번에 모든 것을 초기화 하려고 보니 value들이 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.

모든 초기화 작업을 하나의 초기화 함수에 넣고 해당 함수를 불러오는 방법이 있겠네요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


return (
<>
<div>
<a href="https://vitejs.dev" target="_blank">
<img src={viteLogo} className="logo" alt="Vite logo" />
</a>
<a href="https://react.dev" target="_blank">
<img src={reactLogo} className="logo react" alt="React logo" />
</a>
</div>
<h1>Vite + React</h1>
<div className="card">
<button onClick={() => setCount((count) => count + 1)}>
count is {count}
</button>
<p>
Edit <code>src/App.jsx</code> and save to test HMR
</p>
</div>
<p className="read-the-docs">
Click on the Vite and React logos to learn more
<Section>
<h1>쿠폰함</h1>
<p>
쿠폰을 받아보세요! 주문할 때마다 쿠폰을 받을 수 있어요! 10개를 모으면
무려 1개가 공짜??
</p>
</>
)
<DefaultButton onClick={handleOrderClick} disabled={isDisabled}>
주문하기
</DefaultButton>
<Line />
<IconsContainer>
{opacityList.map((opacity, index) => (
<CoffeeIcon
key={index}
src="CoffeeIcon.png"
alt="커피 아이콘"
style={{ opacity }}
/>
Comment on lines +45 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

style에 직접적으로 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.

브라우저에서 F12로 아이콘을 눌러보니 style="opacity: 0.4;"라는 코드가 있고 주문하기를 누르면 0.4가 1로 바뀌는데 이 부분에 문제가 있을까요?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

css에는 우선순위라는 것이 있어요. 저렇게 되면추후에 바꿀 때 어디인지 찾아봐야 한다는 단점이 생기는 것 같아요. 왜냐하면 inline style은 최고 우선순위 이기 때문이에요. 그렇기 때문에 대부분 개발자들은 class를 많이 사용합니다.
style로 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.

클래스명을 coffee-icon, coffee-icon-inactive로 활성화 비활성화일때 클래스명을 다르게 주고, css에서 클래스명에 따른 스타일을 주면 될 거 같습니다!

))}
</IconsContainer>
<DefaultButton onClick={handleSubmitCoupon} disabled={!isDisabled}>
쿠폰 제출하기 💫
</DefaultButton>
</Section>
);
}

export default App
export default App;

const Section = styled.div`
display: flex;
flex-direction: column;
align-items: center;
margin: 60px auto;
text-align: center;
`;

const Line = styled.div`
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 IconsContainer = styled.div`
display: flex;
justify-content: center;
margin: 30px 0;
`;

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라는 문제점이 있군요! 직접 사용하면서 해당 현상을 겪어보진 않았는데, 추후에 해당 문제를 직접 겪어보도록 실험해봐야겠습니다!

`;
29 changes: 29 additions & 0 deletions src/components/DefaultButton.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import styled from "styled-components";

const DefaultButton = ({ children, disabled, onClick }) => {
return (
<OrderButton onClick={onClick} disabled={disabled}>
{children}
</OrderButton>
);
};

export default DefaultButton;

const OrderButton = styled.button`
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 관련 코드 있는 것과 없는 것의 차이가 분명히 있었어서 코드 넣었던거 같네요!

display: block;
&:hover {
background-color: #366fc9;
}
Comment on lines +23 to +25
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가 어렵답니다...ㅠㅠ

&:disabled {
opacity: 0.5;
}
`;
68 changes: 0 additions & 68 deletions src/index.css
Original file line number Diff line number Diff line change
@@ -1,68 +0,0 @@
:root {
font-family: Inter, system-ui, Avenir, Helvetica, Arial, sans-serif;
line-height: 1.5;
font-weight: 400;

color-scheme: light dark;
color: rgba(255, 255, 255, 0.87);
background-color: #242424;

font-synthesis: none;
text-rendering: optimizeLegibility;
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
}

a {
font-weight: 500;
color: #646cff;
text-decoration: inherit;
}
a:hover {
color: #535bf2;
}

body {
margin: 0;
display: flex;
place-items: center;
min-width: 320px;
min-height: 100vh;
}

h1 {
font-size: 3.2em;
line-height: 1.1;
}

button {
border-radius: 8px;
border: 1px solid transparent;
padding: 0.6em 1.2em;
font-size: 1em;
font-weight: 500;
font-family: inherit;
background-color: #1a1a1a;
cursor: pointer;
transition: border-color 0.25s;
}
button:hover {
border-color: #646cff;
}
button:focus,
button:focus-visible {
outline: 4px auto -webkit-focus-ring-color;
}

@media (prefers-color-scheme: light) {
:root {
color: #213547;
background-color: #ffffff;
}
a:hover {
color: #747bff;
}
button {
background-color: #f9f9f9;
}
}
14 changes: 5 additions & 9 deletions src/main.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
import React from 'react'
import ReactDOM from 'react-dom/client'
import App from './App.jsx'
import './index.css'
import React from "react";
import ReactDOM from "react-dom/client";
import App from "./App.jsx";
import "./index.css";

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