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

3조 과제 제출 (김다슬, 김준희, 임승이) #4

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

Conversation

dev-junehee
Copy link

@dev-junehee dev-junehee commented Aug 10, 2023

🏥 닥터캘 (Dr. Cal)


대학병원 의사들을 위한 쉽고 빠른 당직/연차 관리 서비스


🪧 프로젝트 소개

개발 기간 : 2023. 07. 24 ~ 2023. 08. 10
USER DEMO : 닥터캘(Dr.Cal)
ADMIN DEMO : 닥터캘(Dr.Cal) 관리자

// USER TEST //
ID: [email protected]
PW: test1234

Client Repo : Client
Admin Repo : Admin
Server Repo : Server


👩🏻‍💻 개발자 소개

김다슬 김준희 임승이
김다슬 김준희 임승이
Main Calendar
Admin 사용자 관리
Admin 회원가입 요청
Admin 당직일정 추가
SideBar
회원가입
마이페이지
Admin 당직변경관리
Admin 당직 일정 추가
MainHeader
로그인
요청내역확인
Admin 연차 신청 관리
Admin 당직 일정 추가
Modals

⚒️ 사용기술 및 개발환경

Development

Config

Deployment

Environment

Cowork Tools


💻 프로젝트 테스트

clone project

$ git clone [email protected]:MINI-TEAM3/client.git
$ git clone [email protected]:MINI-TEAM3/admin.git

go to project

$ cd client
$ cd admin

install npm

$ npm install

start project

$ npm run dev

@dev-junehee dev-junehee self-assigned this Aug 10, 2023
kse-seong-eun added a commit that referenced this pull request Aug 11, 2023
<DataCard>
<div className="dataTitle">이메일</div>
<div className="dataText">{duty.email}</div>
</DataCard>
Copy link

Choose a reason for hiding this comment

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

DataCard가 중복되는 것 같은데 내부 텍스트들을 배열로 만든 후 map으로 렌더링하거나 하나의 컴포넌트로 분리해도 좋을 것 같습니다 :)!


const destination = `/${type}`;

const handleOnClickYes = async () => {
Copy link

Choose a reason for hiding this comment

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

저는 개인적으로 이번 프로젝트에서 if-else를 줄이기 위해 신경써봤는데 nesting을 줄이기 위해 early return을 활용하면 코드가 짧아지는 효과가 있어 이런식으로 바꿔보는 방법도 공유하면 좋을 것 같아 한 번 예시로 작성해봤습니다 :)!

const handleOnClickYes = async () => {
  if(!typeof type === 'number'){
    closeModal();
    navigate(destination);
    location.reload();
    return
  }
  await cancelAnnual(type, { id: type })
  closeModal();
}

},
};

export const getLevel = (level: string) => {
Copy link

Choose a reason for hiding this comment

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

export const getLevel = (level: string) => {
  const doctorLevels = {
    PK: '본과실습생',
    INTERN: '인턴',
    RESIDENT: '전공의',
    FELLOW: '전문의'
  }
  return doctorLevels[level]
}

다음과 같이 객체 리터럴을 활용하거나 Map을 활용하면 코드가 더 한눈에 들어올 수 있을 것 같습니다!

import Duty from '@/pages/Duty';
import Register from '@/pages/Register';

const router = createBrowserRouter([
Copy link

@hookor hookor Aug 15, 2023

Choose a reason for hiding this comment

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

createBrowserRouter를 활용해 페이지 구조를 children내에 직관적으로 적으셔서 코드만 보고도 전체적인 구조가 쉽게 이해가능하네요 :) 👍👍

}
};

export const getEvaluation = (eveluation: string) => {
Copy link

@hookor hookor Aug 15, 2023

Choose a reason for hiding this comment

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

Map을 사용하면 들여쓰기를 줄일 수 있을 것 같아 함수가 직관적으로 바뀔 것 같습니다 :)! (evaluationMap을 map으로 간략히 써도 될 것 같은데 이 부분은 명시적인 게 나을 것 같아 그냥 적어봤습니다! 작명에 좋은 의견있으시면 편히 말씀주세요😁😁!!)

export const getEvaluation = (evaluation: string) => {
 const evaluationMap = new Map();
 evaluationMap.set('STANDBY', '대기');
 evaluationMap.set('APPROVED', '승인');
 evaluationMap.set('REJECTED', '반려');
 evaluationMap.set('CANCELED', '취소')
 return evaluationMap.get(evaluation)
}

Copy link

@hookor hookor left a comment

Choose a reason for hiding this comment

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

안녕하세요 3조 여러분 :)!! 발표회 때 짧은 시간에도 전반적인 퀄리티가 높아 인상깊게 지켜봤는데 이렇게 리뷰를 할 수 있는 기회가 생겨 기쁘네요 😊😊!! 이번 프로젝트 수고 많으셨고 다음 프로젝트에도 혹시 만나게 되면 같이 즐겁게 해봤으면 좋겠네요! 수고 많으셨습니다!!!!!!!!@#@#@@#@@!!!@!#!$!$!!!!👍👍👍👍👍👍

Comment on lines +1 to +4
import { ModalType, modalState } from '@/states/stateModal';
import { useCallback } from 'react';
import { useRecoilState } from 'recoil';

Choose a reason for hiding this comment

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

기능이 반복되는 부분을 따로 커스텀훅으로 만들어서 적용신것 같네요! 재사용성을 고려한 좋은 방식 같습니다. 저도 따로 한번 만들어보도록 해야겠습니다~

Comment on lines +19 to +28
const excelDownload = async () => {
const ws = utils.aoa_to_sheet([
[`${hospitalName}_전체_휴가/당직표_${dayjs().format('YYYYMMDD')}`],
[``],
[`유형`, `파트`, `이름`, `직급`, `시작날짜`, `종료날짜`],
]);

data.map(item => {
utils.sheet_add_aoa(
ws,

Choose a reason for hiding this comment

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

엑셀 다운로드를 프론트에서 직접 구현하신건가요? 프론트에서 엑셀 구현하려면 엑셀 표에 나타나는 데이터를 정리한 표 형식으로 코드를 적어야 하는 걸로 알고 있는데 이런 방법도 있는건가 궁금합니다!

Comment on lines +84 to +93
export const editPassword = async (body: editPasswordBody) => {
try {
const res = await instance.post('/user/updatePassword', body, {
headers: {
Authorization: `${localStorage.getItem('authToken')}`,
},
});
return res.data;
} catch (error) {
console.log('비밀번호 변경 실패', error);

Choose a reason for hiding this comment

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

매 api마다 헤더값에 token 로직을 쓰셨네요~ 위에 공통변수로 header token 값에 대한 로직을 작성하셨으면 조금더 깔끔할것 같습니다!

Comment on lines +1 to +5
import { atom } from 'recoil';
import { recoilPersist } from 'recoil-persist';

const { persistAtom } = recoilPersist();

Choose a reason for hiding this comment

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

데이터가 중복으로 필요한곳에 상태관리 라이브러리를 이용하여 적절하게 잘 쓰신것 같습니다. :)

Copy link

@hahahaday12 hahahaday12 left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다~ 전반적으로 컴포넌트 구조를 용도별로 되게 잘 나누신것 같아서 코드 보면서 많이 배웠습니다 ^^

@fronttemp
Copy link

미니 프로젝트 수고 많으셨습니다. 주제 선정도 아주 인상 깊었던 거 같습니다 ㅎㅎ 코드 보면서 코드리뷰를 할려고 했는데 많이 배우고 갑니다.

Copy link

@jungHyeonS jungHyeonS left a comment

Choose a reason for hiding this comment

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

3조 여러분들 미니프로젝트 하시느라 고생많으셨습니다~
컴포넌트 분리도 깔끔하게 잘해주셨고 프로젝트에 필요한 라이브러리도 적극적으로 활용해주신거같습니다~
엑셀 구현도 어려우셨을텐데 깔끔하게 작성해주신거같습니다
다만 중복된 코드들이 몇개 보이고 복잡한 로직 있는 함수들에 대해서 렌더링 성능이 조금 걱정되는 부분이 있던거같습니다 다음 프로젝트에서는 이번에 리뷰 드린 내용 과 react-query를 적극적으로 활용해주시면 좋을꺼같습니다~

Comment on lines +6 to +20
const approveDuty = async (scheduleId: number) => {
if (confirm('승인 처리 하시겠습니까?')) {
const body = {
evaluation: 'APPROVED',
};
await schedule(scheduleId, body)
.then(res => {
if (res.success) {
alert('승인 처리가 완료되었습니다.');
location.reload();
}
})
.catch(error => console.error('당직 승인 실패', error));
}
};

Choose a reason for hiding this comment

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

async/await 과 then/catch 문을 동시에 사용하는것은 비효율적인 코드입니다
async/await 만 사용하는것을 추천드리며 async/await 사용시 try/catch 로 예외처리를 해주시면 됩니다~

const rejectDuty = async (scheduleId: number) => {
if (confirm('반려 처리 하시겠습니까?')) {
const body = {
evaluation: 'REJECTED',

Choose a reason for hiding this comment

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

evaluation 의 값이 REJECTED | APPROVED 등등 다양한 컴포넌트에 나오는거 같습니다 이러한 값은 상수 파일로 관리해주시면 유지보수에 더 용이합니다~

onPageChange: (pageNumber: number) => void;
}

const Pagenation: React.FC<PagenationProps> = ({ totalPage, currentPage, onPageChange }) => {

Choose a reason for hiding this comment

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

최근에는 React.FC 문법은 사용하지 않는 추세 라서 추후에는 React.FC 는 사용하지 않아도 될꺼같습니다~

Comment on lines +14 to +19
const handleClickLogout = async () => {
await logout();
localStorage.removeItem('authToken');
localStorage.removeItem('recoil-persist');
navigate('/');
};

Choose a reason for hiding this comment

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

이부분에서 비동기 통신 실패에 대해 처리주시면 좋을꺼같습니다(로그아웃은 거의 실패 할일은 없지만 그럼에도 불구하고 비동기 통신에 예외처리를 해주시는 습관을 길러주시면 좋습니다)

Comment on lines +107 to +114
{arrDuty.length > 0 ? (
<Duty onClick={() => handleClickDuty(dateObj)}>
<span className="duty-name">• {arrDuty[0]}</span>
<span>{getLevel(arrDuty[1])}</span>
</Duty>
) : (
''
)}

Choose a reason for hiding this comment

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

이부분에 조건부 렌더링은 아래처럼 개선할수있을꺼같습니다~

{ arrDuty.length > 0 && (
  <Duty onClick={() => handleClickDuty(dateObj)}>
    <span className="duty-name">• {arrDuty[0]}</span>
    <span>{getLevel(arrDuty[1])}</span>
  </Duty>
)}

Comment on lines +19 to +33
const handleClickDuty = (date: dayjs.Dayjs) => {
const clickDate = date.format('YYYY-MM-DD');

setModalContent('duty');
setModalOpen(true);
setmodalDate(clickDate);
};

const handleClickAnnual = (date: dayjs.Dayjs) => {
const clickDate = date.format('YYYY-MM-DD');

setModalContent('annual');
setModalOpen(true);
setmodalDate(clickDate);
};

Choose a reason for hiding this comment

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

해당 클릭 함수를 보시면 로직이 동일하신거같습니다 이러한 부분을 파라미터를 하나 추가해서 함수를 합쳐주시면 좋을꺼같습니다~

const handleClick = (type: 'duty' | 'annual', date: dayjs.Dayjs) => {
    const clickDate = date.format('YYYY-MM-DD');

    setModalContent(type);
    setModalOpen(true);
    setmodalDate(clickDate);
};

Comment on lines +24 to +36
useEffect(() => {
const savedSort = localStorage.getItem('usersSort');
if (savedSort) {
setSort(savedSort);
}
getSortedList(0, savedSort || sort);
}, []);

const getSortedList = async (page: number, selectedSort: string) => {
const data = await users({ page: page, sort: `${selectedSort}` });
setUserList(data.item);
setTotalPages(data.totalPages);
};

Choose a reason for hiding this comment

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

이부분이 admin/src/pages/Requests.tsx 에 로직과 매우 유사해보입니다
이러한 중복된 코드를 커스텀 훅으로 만들어주시면 성능 과 코드 가독성이 높아질수있습니다~

Comment on lines +62 to +84
const mapToDate = (dateArray: dayjs.Dayjs[]) => {
return dateArray.map((date, index) => {
const dateObj = dayjs(date);
const arrAnnual = [];
const arrDuty: string[] = [];

Object.keys(calendarData).map(item => {
const index = parseInt(item, 10);
const cal = calendarData[index];
//휴가 출력
if (cal.category === 'ANNUAL') {
if (dayjs(cal.endDate).diff(cal.startDate, 'day') > 0) {
const diffInDays = dayjs(cal.endDate).diff(cal.startDate, 'day');

const dateRange = [];
for (let i = 0; i <= diffInDays; i++) {
dateRange.push(dayjs(cal.startDate).add(i, 'day').format('YYYY-MM-DD'));
}

dateRange.map(item => {
if (item === dateObj.format('YYYY-MM-DD')) {
arrAnnual.push(item);
}

Choose a reason for hiding this comment

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

이 함수를 보시면 로직이 상당히 복잡해보이는데 이러한 함수를 useMemo 혹은 useCallback으로 묶어주시면 좋습니다 또한 하나의 함수에 많은 기능을 넣기보다는 함수 안에서도 코드를 분리하는데 신경써주시면 좋습니다~

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.

5 participants