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.

Sign up for GitHub

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

실험실 - 다중 판매 문구 변경 #273

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+73 −14
Open

실험실 - 다중 판매 문구 변경 #273

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65f6eee
fix: 실험실- 다중 판매 문구 변경
HoberMin Jan 10, 2025
c364cae
feat: 펫 판매 기능 다국어 지원 추가
HoberMin Jan 13, 2025
251a443
refactor: DropPetsResult 컴포넌트의 번역함수 호출 최적화
HoberMin Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion apps/web/messages/en_US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,10 @@
"property-pet-sell": {
"title": "Property Pet Sell",
"description": "Sell your pet to other users!",
"count": "ea"
"count": "ea",
"saleSuccess": "sale success",
"saleFail": "sale fail",
"totalAmount": "Total Amount"
}
},
"Inbox": {
Expand Down
9 changes: 6 additions & 3 deletions apps/web/messages/ko_KR.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,12 @@
},
"Laboratory": {
"property-pet-sell": {
"title": "Property Pet Sell",
"description": "Sell your pet to other users!",
"count": "개"
"title": "펫 판매",
"description": "보유한 펫을 판매해보세요!",
"count": "개",
"saleSuccess": "판매 완료",
"saleFail": "판매 실패",
"totalAmount": "총 판매 금액"
}
},
"Inbox": {
Expand Down
65 changes: 55 additions & 10 deletions apps/web/src/app/[locale]/laboratory/property-pet-sell/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,63 @@ const PersonaList = wrap
});
};

const DropPetsResult = ({ success, errors }: { success: { givenPoint: number }[]; errors: unknown[] }) => {
const t = useTranslations('Laboratory.property-pet-sell');

if (success.length === 0 && errors.length === 0) {
return (
<div>
<p>
0{t('count')} {t('saleSuccess')}, 0{t('count')} {t('saleFail')}
</p>
</div>
);
}

if (success.length === 0) {
return (
<div>
<p>
{errors.length}
{t('count')} {t('saleFail')}
</p>
Copy link
Member

Choose a reason for hiding this comment

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

const errorCount = error.length;
const 개 = t('count');
const 실패 = t('saleFail');

...
return <p>{errorCount}{개} {실패}</p>

이런 형태는 어떻게 생각하시나요?

  • errorLength

    • 현재는 단순히 배열의 길이로 판단하고 있지만, 이후 API 스펙이 변경되어 성공 객체의 특정 프로퍼티 기준으로 분기를 나눠야할 경우 대처가 쉬워진다고 생각하고 && 미약하지만 매번 프로토타입 체이닝을 하는 것보다 성능상 우위를 가질 수 있어요

  • const 개 = t('count');

    • 읽기 좋은 코드에 가까워진다고도 생각이 들며, 위와 마찬가지로 매번 접근하는 것보다 성능상 우위가 있다고 생각해요

단순히 제 생각뿐이며, 의견 부탁드려요 ~ 👍

Copy link
Author

Choose a reason for hiding this comment

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

코드 가독성면에서 고려를 하지 못했네요 ,, 성능적인 부분에 있어서는 우위가 있다는걸 처음알게 되었습니다. !
덕분에 많은 내용을 배우고 있습니다. 감사합니다.

해당 부분은 count saleSuccess saleFail totalAmount네가지로 변수 선언해서 재사용했습니다. 👍

Copy link
Member

@hyesungoh hyesungoh Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

작업해주셔서 제가 더 감사드리죠! 👍

추가적으로 궁금한 점이, 제가 영어에 약해서 그런지 몰라도 개와 count 중 읽기 자연스러운 건 개라고 생각이 드는데요

저보다 더 많이 고민해보셨을, 작업자 분께서 영어로 네이밍하신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

변수명을 한글로 선언하는게 습관이 안되어있어서 개라고 남겨주셨음에도 count를 선택했습니다. 혹시 코드컨벤션이나 이런것들을 고려했을 때 개라는 변수명을 사용하는것이 더 직관적이실까요??

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
Author

Choose a reason for hiding this comment

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

의도했던 작업은 모두 완료된 것 같습니다 👍
혹시, 또 다른 이슈를 발견하면 이슈에 남기고 기여를 요청드려도 괜찮을까요 ??
현재 기여를 계속 받을 계획이 있으신지에 대해 알려주신다면 감사하겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

@hyesungoh 혹시 추가작업 필요한게 있을까요 ??

Copy link
Member

Choose a reason for hiding this comment

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

아 제가 정신이 없어서 요즘, 이제야 봤네요 ㅎ

네 추가 기여는 너무 감사드리죠!

현재 PR에서 추가 작업이 필요해 보이는 건 없고, @sumi-0011 님이 확인해주신 후 머지 부탁드려요!

</div>
);
}

const totalPrice = success.reduce((acc, curr) => acc + curr.givenPoint, 0);

if (errors.length === 0) {
return (
<div>
<p>
{success.length}
{t('count')} {t('saleSuccess')}
</p>
<p>
{t('totalAmount')}: {totalPrice}P
</p>
</div>
);
}

return (
<div>
<p>
{success.length}
{t('count')} {t('saleSuccess')}, {errors.length}
{t('count')} {t('saleFail')}
</p>
<p>
{t('totalAmount')}: {totalPrice}P
</p>
</div>
);
};

const onSell = async (ids: string[]) => {
const res = await dropPets({ personaIds: ids });

const totalPrice = res.success.reduce((acc, curr) => acc + curr.givenPoint, 0);

trackEvent('laboratory', {
type: '레벨, 타입 같은 펫 한번에 팔기',
});
Expand All @@ -62,14 +114,7 @@ const PersonaList = wrap

showDialog({
title: '펫 판매 완료',
description: (
<div>
<p>
{res.success.length}마리 판매 완료, {res.errors.length}마리 판매 실패
</p>
<p>총 판매 금액: {totalPrice}P</p>
</div>
),
description: <DropPetsResult success={res.success} errors={res.errors} />,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

다이얼로그 개선이 필요합니다

다이얼로그에 다음과 같은 개선사항들이 있습니다:

  1. 다이얼로그 제목이 하드코딩되어 있습니다
  2. 판매 중 로딩 상태 표시가 없습니다
  3. API 호출 에러 처리가 누락되었습니다

다음과 같이 개선해보세요:

 const onSell = async (ids: string[]) => {
+  const t = useTranslations('Laboratory.property-pet-sell');
+  try {
+    showDialog({
+      title: t('selling-pets'),
+      description: t('selling-in-progress'),
+      showConfirmButton: false,
+    });
+
     const res = await dropPets({ personaIds: ids });
 
     trackEvent('laboratory', {
       type: '레벨, 타입 같은 펫 한번에 팔기',
     });
 
     queryClient.invalidateQueries({ queryKey: userQueries.allPersonasKey() });
 
     showDialog({
-      title: '펫 판매 완료',
+      title: t('sale-complete'),
       description: <DropPetsResult success={res.success} errors={res.errors} />,
     });
+  } catch (error) {
+    showDialog({
+      title: t('sale-error'),
+      description: t('sale-error-description'),
+    });
+  }
 };

Committable suggestion skipped: line range outside the PR's diff.

});
};

Expand Down