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

migrate exam subsection, share subsection to ts #74

Open
wants to merge 3 commits into
base: migration
Choose a base branch
from

Conversation

useruseruse
Copy link
Contributor

No description provided.

@useruseruse useruseruse self-assigned this Jul 17, 2024
from: LectureFocusFrom;
clicked: boolean;
lecture: Lecture | null;
reviews: Review[] | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shapes/ 폴더의 lectureFocus type 로 통일하였음.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.

Project coverage is 6.07%. Comparing base (6076b84) to head (6c163ea).

Files Patch % Lines
...ns/timetable/timetableandinfos/ShareSubSection.tsx 0.00% 25 Missing ⚠️
src/redux/actions/timetable/lectureFocus.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           migration     #74      +/-   ##
============================================
+ Coverage       5.93%   6.07%   +0.14%     
============================================
  Files            214     211       -3     
  Lines           6574    6415     -159     
  Branches        1745    1678      -67     
============================================
  Hits             390     390              
+ Misses          6023    5872     -151     
+ Partials         161     153       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

</div>
<div className={classNames('subsection--exam__content')}>
<Scroller>
{[0, 1, 2, 3, 4].map((dayIndex) => (
Copy link
Member

Choose a reason for hiding this comment

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

day enum 있으면 가져다 써도 좋을 듯

Suggested change
{[0, 1, 2, 3, 4].map((dayIndex) => (
{Object.values(DayEnum).map((dayIndex) => (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

요거 좋을 것 같아서 찾아봤는데
const Enum 으로 선언된 값은 실제 런타임에서 javascript 객체로 존재하지 않고 인라인으로 치환되어서, Object.values(DayEnum) 과 같이 사용할 수 없는 것 같아요.
대신에 요런 식으로는 바꿀 수 있을 것 같아요. 저는 명시적으로 어떤 enum 인지 알려주는 게 좋은 것 같은데, 어떤게 좋을지 의견 주시면 감사하겠습니다!

{[Day.MON, Day.TUE, Day.WED, Day.THU, Day.FRI].map((dayIndex) => (

참고
image

Copy link
Member

@yumincho yumincho Aug 7, 2024

Choose a reason for hiding this comment

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

enum 관련 논의가 전에 있었던 것 같은데..Day enum 포함해서 enum.ts에서 관리하던 enum들 as const로 옮기는 게 나을까요?

cf. [타입 스크립트 핸드북] Objects vs. enums

Copy link
Contributor Author

Choose a reason for hiding this comment

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

옹 넵.! 체킹이 아니라 런타임에서 enum 을 많이 가져다 써서 객체 - as Const 로 하는게 좋을 것 같은데, 오늘 회의시간에 논의하고 확정하면 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

좋슴다 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#73
요기에서 수정이 되었는데,
as Const 로 바꾸니 지금Semester.Spring이런 식으로 되어 있는 것들을 모두 Semester['Spring'] 이렇게 바꾸어야 해서 default value 를 할당하는 방향으로 바꾸었습니당

isLectureListOpenOnMobile ? 'mobile-hidden' : null,
)}>
<div>
{user && selectedTimetable && year && semester ? (
Copy link
Member

Choose a reason for hiding this comment

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

앞에서 조건부 return을 추가하는 게 더 깔끔해보일 것 같네요

if (!user && selectedTimetable && year && semester) {
return <> ... </>
}

return <> ... </>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

음 넵 조건문을 그냥 따로 빼도 좋을 것 같구,
const Condition = user && selectedTimetable && year && semester
대충 공통 컴포넌트 끼리 빼서 리팩토링해도 좋을 것 같아용

  const ShareBlock = ({disabled, icon }) => {
    const display_str = `ui.button.${icon}`;
.... 대충...
    return (
      <div className={classNames('subsection--share__item')}>
        <span className={classNames('disabled')}>
          <i className={classNames('icon', 'icon--share-syllabus')} />
          <span>{t(display_str)}</span>
        </span>
      </div>
    );
  };

Copy link
Member

@yumincho yumincho Aug 7, 2024

Choose a reason for hiding this comment

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

skeleton 컴포넌트 만드는 방향으로 이해하고 있는데, 태스크/PR 분리 하면 좋겠어요!

cf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

요 컴포넌트는 지금은 disabled이거나 enabled 인 상태만 있고 유저 로딩중에는 그냥 disabled 처리가 됩니다!

이 부분은
현재는 아래와 같이 처리하고 있는 코드가 몇군데 있는데,
같은 컴포넌트이고, 컴포넌트 상태 disabled/enabled/loading/hover/...etc 에 따라서 ui 적으로 살짝만(색깔 등) 변하는 경우에도 부모에서 분기를 하는 방식으로 작성이 되어 있어서,

// 부모 컴포넌트에서 
isDisabled?    
<Disable시에 보여줄 무슨무슨 컴포넌트>
: <Enable시에 보여줄 무슨무슨 컴포넌트>

아래와 같이 분리하는게 좋을 것 같은데,

// 자식 컴포넌트에서  를 isDisabled  prop 으로 받고, 이에 따라 다르게 렌더링
// 부모 컴포넌트에서는 하나의 자식 컴포넌트에 prop 으로 isDisabled 만 내려줌. (조건 연산 x) 

말씀하신것처럼 태스크 분리를 하는 게 좋아보여서 다른 PR 에서 처리하거나,
TODO 로 남겨놓고 이런 부분들은 나중에 한꺼번에 고치면 어떨까 싶습니닷.

src/redux/actions/timetable/lectureFocus.ts Outdated Show resolved Hide resolved
src/redux/reducers/timetable/lectureFocus.ts Show resolved Hide resolved
src/utils/lectureUtils.ts Outdated Show resolved Hide resolved
src/utils/lectureUtils.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants