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

feat: 사서 대출권수 변경 #620

Merged
merged 19 commits into from
Sep 27, 2024

Conversation

hyeunkim42
Copy link
Contributor

@hyeunkim42 hyeunkim42 commented Sep 14, 2024

개요

사서의 대출권수를 4권으로 변경했습니다. (사서의 특권)
사서 본인이 본인의 대출을 진행할 때만 4권까지 가능합니다. 이는 앞으로 개발 될 예정인 집현전의 무인화 및 코드작성 편의를 고려하여 설정한 조건입니다.

관련 이슈

#619

추가 기능

  • 대출 진행 시 사서 표시하는 UI 추가
스크린샷 2024-09-20 오전 11 58 59
  • 대출 확인 모달의 도서 간 경계 및 비고란의 배경색 추가
스크린샷 2024-09-20 오후 3 13 02

@hyeunkim42 hyeunkim42 linked an issue Sep 14, 2024 that may be closed by this pull request
5 tasks
lendingLimit 변수의 값이 selectedUser가 librarian(현재 로그인한 유저)인 경우 4로 설정됨
- prop으로 넘겨받고 있는데, Atom 에서 받아와서 설정하도록 다시 변경해야함
setSelectedBooks내부에서 불러오는 함수를  splice에서 filter로 변경
@hyeunkim42 hyeunkim42 closed this Sep 19, 2024
@mixsung mixsung added 사서 UI/UX UI/UX 관련 labels Sep 20, 2024
@hyeunkim42 hyeunkim42 changed the title WIP WIP: 사서 대출권수 변경 Sep 20, 2024
@hyeunkim42 hyeunkim42 changed the title WIP: 사서 대출권수 변경 wip: 사서 대출권수 변경 Sep 20, 2024
@hyeunkim42 hyeunkim42 marked this pull request as ready for review September 21, 2024 06:37
@hyeunkim42 hyeunkim42 marked this pull request as draft September 21, 2024 06:38
TextareaWithLabel에서 setState를 다시 생성해서 관리하고 있던 부분을 받아온 state를 이용하도록 변경
@mixsung mixsung changed the title wip: 사서 대출권수 변경 feat: 사서 대출권수 변경 Sep 21, 2024
@mixsung mixsung marked this pull request as ready for review September 21, 2024 14:57
selectedUser.isPenalty && (penalty += "대출제한 (연체");

// 제한 권수 판단
const lendingLimit = librarian && librarian.id === selectedUser.id ? 4 : 2;
Copy link
Member

@YeonSeong-Lee YeonSeong-Lee Sep 22, 2024

Choose a reason for hiding this comment

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

34번 라인같은 코드가 많이 보이는데 함수도 빼면 가독성이 더 좋을거 같아요. util함수 같은 느낌으로다가 constant 폴더에 넣으면 적합하다고 생각합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

constant의 status.js에 함수화 시켜서 넣어봤습니다. 해당 함수에 객체를 인자로 넘기는데, 객체의 요소(isAdmin이나 id)가 undefined 같은 빈 값으로 들어오는 것도 고려해야 좋은지 고민이 됩니다.

Copy link
Member

@YeonSeong-Lee YeonSeong-Lee Sep 27, 2024

Choose a reason for hiding this comment

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

이 코멘트를 늦게 봤네요..

아래 제안사항과 같이 옵셔널 체이닝을 이용하면 undefined에서 접근할 때 터지는 걸 예방할 수 있어서 옵셔널 체이닝으로 수정해두고,

값 검증이 필요하다면 타입을 추후에 추가하거나 하면 좋을 거 같습니다.

https://github.com/jiphyeonjeon-42/frontend/pull/620/files#r1778081559

src/component/rent/RentModalConfirm.tsx Outdated Show resolved Hide resolved
src/component/rent/RentModalUserList.tsx Outdated Show resolved Hide resolved
src/component/rent/Rent.tsx Show resolved Hide resolved
mixsung and others added 3 commits September 23, 2024 17:40
피드백 반영해서
1. 변수 이름을 librarian에서 currUser로 수정했습니다.
2. lending limit의 경우 constant 폴더의 status에서 가져와서 사용하도록 했습니다.
undefined 접근 에러를 예방하기 위해 옵셔널 체이닝을 추가합니다.

추후 필요에 따라 리팩토링과정에서 타입이 추가 될 수 있습니다.
@YeonSeong-Lee YeonSeong-Lee merged commit d499d5a into develop Sep 27, 2024
5 checks passed
@YeonSeong-Lee YeonSeong-Lee deleted the 619-사서-대출권수-4권으로-변경 branch September 30, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI/UX UI/UX 관련 사서
Projects
None yet
Development

Successfully merging this pull request may close these issues.

사서 대출권수 4권으로 변경
3 participants