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

[혜택관리] 혜택 문구 추가, 수정 기능 추가 #72

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

chaeseungyun
Copy link
Contributor

혜택 관리 페이지에서 혜택 정보를 추가하고 수정할 수 있는 기능을 추가했습니다

기존과 다르게 테이블 뷰로 보이도록 변경했습니다

혜택 정보 값을 입력하지 않으면 추가, 수정할 수 없도록 만들었습니다.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

detail: string;
}[];
}

export interface ModifyBenefitForm extends CreateBenefitRequest { }

Choose a reason for hiding this comment

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

코드 검토 결과, 다음과 같은 주요 사항과 개선점을 확인했습니다:

  1. 인터페이스 이름 통일성: ShopsShopInfo의 관계를 명확히 하기 위해 ShopInfoShops를 확장하는 형태로 수정되었습니다. 이는 데이터의 의미를 명확히 하고 혼동을 줄일 수 있습니다.
  2. AddShopRequest의 구조 개선: AddShopRequest에서 DeleteShopsRequest를 상속받지 않는 것을 권장합니다. 대신 새로운 필드인 shop_details를 추가하여 이 요청의 목적을 더 분명하게 나타냅니다.

아래는 개선이 필요한 부분의 diff 형식입니다:

-  shops: Shops[];
+  shops: ShopInfo[];

 export interface Shops {
   id: number;
   name: string;
 }
 
+export interface ShopInfo extends Shops {
+  shop_benefit_map_id: number;
+  detail: string;
+}

-export interface AddShopRequest extends DeleteShopsRequest {}
+export interface AddShopRequest {
+  id: number;
+  shop_details: ShopDetail[];
+}

이와 같은 개선 사항을 통해 코드의 가독성과 유지보수성을 높일 수 있으며, 각 데이터 구조에 대한 명확한 이해를 도울 수 있습니다.

height: 100%;
border: none;
outline: none;
`;

Choose a reason for hiding this comment

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

코드 리뷰 결과에 따라 몇 가지 개선 사항을 제안합니다.

  1. 주석 추가: SelectContainer가 주석처리된 이유가 명확하지 않습니다. 이 부분에 대한 설명이 필요할 수 있습니다.

  2. CSS 속성을 사용하는 방식: ButtonWrapperdisplay 속성을 relative로 수정하는 대신 position 속성을 사용한 것은 올바른 절차입니다. 그러나 CSS 속성에서 display: relative는 존재하지 않으므로 이를 position: relative로 수정한 것은 좋은 개선입니다.

  3. 부정확한 위치값: DeleteButtonWrappertop 값을 -10px에서 -15px으로 조정한 것은 구체적인 목표에 따라 차이가 있을 수 있지만, 이 변경이 적절한지에 대한 주석이 필요할 것 같습니다.

  4. 입력 필드 스타일링: DetailInput에 대한 스타일이 새로 추가되었습니다. 이 스타일은 사용에 적합해 보이나, 해당 입력 필드가 어디에 사용될지에 대한 설명이 주석으로 추가될 수 있다면 더욱 좋을 것입니다.

다음은 개선이 필요한 부분에 대한 diff 형식 제안입니다:

@@ -1,6 +1,7 @@
 /*
- SelectContainer는 현재 사용되지 않습니다. 필요에 따라 다시 활성화할 수 있습니다.
+ SelectContainer가 주석처리된 이유에 대한 설명이 필요합니다.
 */
-export const SelectContainer = styled.div`
+// export const SelectContainer = styled.div`
 
@@ -54,12 +55,12 @@ export const ButtonContent = styled.span`
-  display: relative; 
+  position: relative; // display 속성 대신 position 속성 사용
 `;
 
 export const DeleteButtonWrapper = styled.button`
   position: absolute;
-  top: -10px;
+  top: -15px; // 위치 조정 필요성에 대한 설명 추가
   left: -10px;
   background: none;
   border: none;
@@ -72,3 +73,10 @@ export const FlexRight = styled.div`
   margin-top: 10px;
 `;
 
+// DetailInput은 특정 폼에 사용될 입력 필드입니다.
 export const DetailInput = styled.input`
   width: 100%;
   height: 100%;
   border: none;
   outline: none;
 `;

각 수정 사항에 주석을 추가하여 코드의 맥락을 명확히 할 수 있도록 했습니다.

Copy link
Contributor

@hoooooony hoooooony 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 +29 to +33
const shopDetail = details.find((shop) => shop.shop_id === shopId);
if (shopDetail) {
const findItem = details.find((shop) => shop.shop_id === shopId);
if (!findItem) return;
const newDetail = {
Copy link
Contributor

Choose a reason for hiding this comment

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

좀 더 깔끔하게 어떻게하면 좋을까요?

@chaeseungyun chaeseungyun merged commit 295e9dd into develop Dec 17, 2024
2 checks passed
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