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

[리뷰용 PR] 탭투탭 구현 및 시간표 관련 코드 전체 리팩토링 #246

Closed
wants to merge 71 commits into from

Conversation

simeunseo
Copy link
Member

@simeunseo simeunseo commented Jun 25, 2024

단기간에 너무 많은 내용을 추가하다보니 하나하나 리뷰받으면서 넘어가기엔 무리가 있어서,
이렇게 모든 변경사항을 한번에 담은 리뷰용 PR을 따로 올립니다.

제가 이번에 진행한 작업들은 다음과 같습니다.
재훈오빠가 담당했던 우선순위 드롭다운 컴포넌트를 제외하고,
시간표와 관련된 모든 부분을 새로 만들었다고 보시면 됩니다.

  • 시간표 컴포넌트 구현 (components/timetableComponents 폴더 참고)
  • 가능 시간 입력과 우선순위 입력을 한 페이지로 묶어서 구현 (pages/selectSchedule 폴더 참고)
  • 종합 일정 시간표 구현 (pages/overallSchedule 폴더 참고)
  • 회의 시간 선택지 GET API, 종합 일정 시간표 GET API에 대해 react-query로 리팩토링

어마어마한 Files changed... 죄송합니다 ㅠ^ㅠ
하지만 기존 코드를 리팩토링하면서 파일을 다른 폴더로 옮기는 등의 작업으로 보지 않아도 될 Files changed가 많습니다. (자꾸 린팅 내용이 추적되는 것도 해결을 봐야겠네요...)

그래서 제가 정리한 아래의 글들을 참고하셔서 구조를 파악하고 필요한 부분의 코드만 보시는 걸 추천드립니다.
어떤 생각과 설계로 이 작업을 진행했는지에 대해 상세히 담은 글들입니다.
1탄 - 명세 정리
2탄 - 가능 시간 입력
3탄 - 재사용

논의가 필요한 사항에 대해서는 Slack 스레드 확인해주세요!

RPReplay_Final1719142760.MP4

simeunseo added 30 commits June 20, 2024 21:51
Copy link
Member

@ljh0608 ljh0608 left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다! 로직이 깔끔해서 이해하기 용이했지만 네이밍관련 부분은 여러 의견을 던져보았는데 이부분은 답변주시면 감사하겠습니다! 그리고 상수파일 분리와 같은 부분 등 은서님이 진행하시지 않았을 부분도 몇개 리뷰를 남겼는데 그러하다면 추후 같이 리팩토링 진행해보면 좋을 것 같습니다~!

Comment on lines 45 to 48
const TimetableWrapper = styled.div`
display: flex;
gap: 0.75rem;
`;
Copy link
Member

Choose a reason for hiding this comment

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

p4) 시맨틱한 태그 사용을 위해 해당부분 section 태그 사용하는거 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

asap이 일반적인 문서 형태의 뷰가 아니다보니 시맨틱 태그를 사용하기에 애매한 부분이 있는 것 같아요...
section을 제안하신 이유는 혹시 무엇일까요?
section은 페이지 내에 여러 하위 주제들을 나눌 때 적절하다고 이해하고있는데
제 생각에 시간표는 페이지내에 가장 주된 내용이니까 main태그는 어떨까 싶습니다!

Copy link
Member

Choose a reason for hiding this comment

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

결론 먼저 말씀드리자면 메인 태그가 의미상 더 알맞을 것 같다는 생각이 드네요!

저는 section태그가 문서 형태의 뷰만 사용되는 것이 아니라 웹페이지의 각 부분의 기능영역, 주제, 역할을 구분 하는데에도 쓰인다고 생각을 했습니다!
또한 문서라는 것이 좁은 의미로 생각하는 것이 아니라 HTML로 이루어진 모든 웹 페이지는 웹 문서라고 생각합니다.
asap서비스도 시간표를 보여주고, 가능 시간 유저를 보여주는 문서라고도 생각이 들었어요! 사실 "web document(웹 문서)"라는 것을 어떻게 정의하냐에 대한 확신이 있는것은 아닙니다만, main태그로 시간표를 감싼다면 아래 bottomItem 부분은 section을 사용하는게 논리적으로 기능을 나누고, 역할을 구분할 수 있다고 생각했습니다! bottomItem의 div는 다른 일반적인 wrapper div와는 다른 의미를 가진다고 생각했기 때문이기도 해요

제 주장을 뒷받침할만한 두가지 링크를 남깁니다!
첫 링크는 web문서의 정의, 두번째 링크는 문서가 무엇인지 고민해볼 수 있는 링크입니다.
https://docs.oracle.com/cd/F82674_01/crm92pbr21/eng/crm/conl/UnderstandingWebDocuments-094e06.html
https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/Document_and_website_structure
image

Copy link
Member Author

@simeunseo simeunseo Jul 1, 2024

Choose a reason for hiding this comment

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

오웅 좋은 자료네요..!! 딥다이브 멋집니다 저도 많은 도움이 되었어요

제가 "일반적인 문서 형태"가 아니라고 언급한 것은 웹 문서가 아니라기보다 전통적인 형태의 웹이 아니라는 의미였어요!
mdn에서도 "typical website"라고 얘기하면서 웹 페이지의 일반적인 구조를 설명하고, <header> <main> <section> <footer>등 여러 시맨틱 태그에 대해서도 이 구조에 기반하여 설명하고 있어요. <section> 태그는 예를 들어 기사의 제목을 기준으로 나눌 때 사용하라고 설명하고 있는 것처럼요.
image

그런 의미에서 시간표 컴포넌트는 ... 상당히 애매합니다!
저도 <main>태그가 어떻냐고 말했다가도 mdn에서는 main태그는 body바로 아래에 있어야한다, 다른 element안에 중첩되어있으면 안된다, 라고 하니 <main>을 써야할 곳은 시간표 컴포넌트가 아니라 그냥 가능시간입력 페이지 자체(더 넓은 영역)가 되어야하지 않나 생각이 들었어요.

어쨌든 문서의 조각을 좀 더 의미있게 표현해보자 라는 취지이니, mdn을 참고 하되 저희 서비스에 맞춰 적용해보자면
결론적으로 시간표 컴포넌트에는 <section> 태그가 어떨까 싶습니다.
시간 입력이라는 특정 기능을 수행하는 요소이면서 페이지 내의 다른 콘텐츠(헤더, 회의설명, 다음 버튼 등)와 구별되는 특정한 주제(시간 입력)를 가지고 있기 때문입니다.

<section> 태그는 페이지의 특정 기능을 하나로 묶는 데 더 적합합니다.

Copy link
Member

Choose a reason for hiding this comment

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

오! 저 역시 비슷한 생각을 했습니다!

  1. 시간표가 main이 아닌 가능 시간입력 페이지 자체가 main이 된다.
  2. 그 안에 두가지 section (시간표 컴포넌트, bottomItem)이 있다

로 이해했는데 맞을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

네넵 동의합니다!

src/components/timetableComponents/parts/SlotTitle.tsx Outdated Show resolved Hide resolved
src/pages/OverallSchedule/OverallSchedule.tsx Show resolved Hide resolved
src/pages/OverallSchedule/components/UserNames.tsx Outdated Show resolved Hide resolved
src/pages/OverallSchedule/components/UserNames.tsx Outdated Show resolved Hide resolved
src/pages/selectSchedule/utils.ts Outdated Show resolved Hide resolved
Copy link
Member Author

@simeunseo simeunseo left a comment

Choose a reason for hiding this comment

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

리뷰내용 모두 확인했습니다 고생 많으셨습니다 🙇🏻‍♀️

src/components/timetableComponents/parts/SlotTitle.tsx Outdated Show resolved Hide resolved
src/pages/OverallSchedule/OverallSchedule.tsx Show resolved Hide resolved
src/pages/OverallSchedule/components/UserNames.tsx Outdated Show resolved Hide resolved
src/pages/OverallSchedule/components/UserNames.tsx Outdated Show resolved Hide resolved
src/pages/selectSchedule/utils.ts Outdated Show resolved Hide resolved
@simeunseo simeunseo requested a review from ljh0608 June 30, 2024 09:37
Copy link
Member

@ljh0608 ljh0608 left a comment

Choose a reason for hiding this comment

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

브랜치 conflict 잡아주셔야할 것 같습니다! 이부분 + yarn build 명령어를 사용해서 빌드에 문제 없게 해주시면 감사하겠습니다!

@simeunseo simeunseo changed the title [리뷰용 PR] 탭투탭 구현 및 시간표 관련 코드 전체 리팩토링 [Feat] 탭투탭 구현 및 시간표 관련 코드 전체 리팩토링 Jul 5, 2024
@simeunseo simeunseo closed this Jul 5, 2024
@simeunseo simeunseo changed the title [Feat] 탭투탭 구현 및 시간표 관련 코드 전체 리팩토링 [리뷰용 PR] 탭투탭 구현 및 시간표 관련 코드 전체 리팩토링 Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants