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

MISSION7 / 곽민준 #45

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

Conversation

mlnwns
Copy link
Collaborator

@mlnwns mlnwns commented May 17, 2024

1. 구현 모습

2. 해결 과정

generateTimes 함수를 사용하여 오전 9시부터 오후 8시까지의 시간대를 생성하고, useSelectedTimes 훅을 사용하여 사용자가 선택한 시간을 관리하도록 했습니다. 사용자가 예약하기 버튼을 클릭하면, 선택된 시간들이 유효한지 검사한 후, 예약이 완료되었다는 알림을 표시합니다. 이 과정에서 선택된 시간들은 정렬되고, formatReservationTime 함수를 통해 포매팅하도록 했습니다.

3. To 리뷰어에게

CSS부분에서 생각보다 어려움을 겪었습니다. 각각의 시간 선택하는 부분에 border를 주니까 border가 옆의 요소와 겹치는 현상이 있어서, border-left만 주고, 큰 상자에서만 border-right를 주었더니 해결되었습니다. 뭔가 어색한 부분이 있는데 더 좋은 방법이 있는지 여쭤보고 싶습니다!
개발을 하면서 대부분 components와 page 폴더만 만들고 코드를 작성했는데, 코드가 지저분해지는 감이 있어서 utils폴더와 hooks폴더를 나누어서 코드를 분리해보았습니다. 잘 분리했는지 위주로 살펴봐주시면 감사하겠습니다!
바쁜 시간 내어 리뷰해주셔서 감사합니다 :)


@mlnwns mlnwns added the mission 미션 입니다! label May 17, 2024
@mlnwns mlnwns requested a review from JUDONGHYEOK May 17, 2024 06:31
@mlnwns mlnwns self-assigned this May 17, 2024
Copy link

vercel bot commented May 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
donut-study ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2024 6:31am

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.

안녕하세요 민준님!! 첫 미션이후로 오랜만에 만나뵙게 되네요!! 처음 보았을 때 민준님의 코드 스타일과 확연하게 달라진 것 같아요. 그 달라진 방향이 제가 생각하기에는 좀 더 읽기 쉬워지고 간결해진 느낌입니다. 민준님의 성장에 제가 함께한 것 같아 저도 기분이 좋네요~!!😃

이번 미션에서는 구조적인 부분에서는 따로 피드백드릴게 없는 것 같아요 간단한 어플리케이션이기는 하지만 hook이나 util분리도 잘해주신것 같구요. 다만 조금 더 간결하게 구성할 수 있을 것 같아요. 개인적으로는 불필요한 로직들이 조금씩 보였던 것 같습니다.

아쉬웠던 점은 변수명이나 스타일 관련해서는 조금 고민이 더 필요한 부분인 것 같습니다. 하지만 지금 상태로도 충분히 좋은 코드를 만들어주셨다고 생각해서 제 의견은 하나의 의견으로 참고해주시고 민준님께서 더 고민을 해보시고 좋은 방향이 있다면 디벨롭 해주시면 감사하겠습니다!! 바쁜 학업생활속에서 미션하시느라 고생많으셨습니다!!

function App() {
const [count, setCount] = useState(0)
const App = () => {
const times = generateTimes(9, 20);
Copy link
Collaborator

Choose a reason for hiding this comment

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

매직넘버는 상수로 분리해주는 것이 좋을 것 같아요!

{!isEnd && (
<>
<StyledEmptyArea></StyledEmptyArea>
<StyledColorArea $isActive={isActive}></StyledColorArea>
Copy link
Collaborator

Choose a reason for hiding this comment

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

개인적으로 굳이 달러사인을 붙이지 않아도 될 것 같아요!


export default TimeSlot;

const StyledTimeSlot = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

clickable한 요소는 button으로 구성하는 것이 좋지 않을까요??

Comment on lines +1 to +12
const generateTimes = (startHour, endHour) => {
const times = [];
for (let hour = startHour; hour <= endHour; hour++) {
times.push(`${hour}:00`);
if (hour !== endHour) {
times.push(`${hour}:30`);
}
}
return times;
};

export default generateTimes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 로직도 훌륭하지만 현재 방식에서는 startHour를 기준으로 데이터를 다루고 있는 . 것같아요. 제 생각에는 저장하는 데이터 형식을 다르게하여 로직들을 간편하게 하면 좋을 같다는 생각인데 민준님은 어떻게 생각하시나요?

Comment on lines +37 to +38
onSelectTime={handleSelectTime}
isActive={selectedTimes.includes(time)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

읽는 사람의 차이이겠지만 onSelectTime과 isActive는 잘 안읽히는 것 같습니다. onSelectTime이라고하면 select 엘리먼트가 생각이나서 여러 선택지중 하나를 고르는 듯한 느낌을 주고 isActive는 위에서는 selected라는 이름이 계속 되고 있는데 isActive가 나오는게 살짝 어색한 것 같아요!


const TimeSlot = ({ time, onSelectTime, isActive }) => {
const isHour = time.endsWith(":00");
const isEnd = time === "20:00";
Copy link
Collaborator

Choose a reason for hiding this comment

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

isEnd를 확인해서 스타일링 하는 것이 뭔가 어색한데요. 다른 방법은 없을까요?

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.

2 participants