실험실 - 다중 판매 문구 변경 #273
실험실 - 다중 판매 문구 변경 #273
Conversation
워크스루이 변경사항은 변경 사항
가능한 관련 PR
제안된 라벨
제안된 리뷰어
시퀀스 다이어그램sequenceDiagram
participant User
participant OnSellFunction
participant DropPetsResult
User->>OnSellFunction: 펫 판매 요청
OnSellFunction->>DropPetsResult: 성공/실패 결과 전달
DropPetsResult-->>User: 판매 결과 표시
토끼의 시 🐰
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const DropPetsResult = ({ success, errors }: { success: { givenPoint: number }[]; errors: unknown[] }) => { | ||
| const totalPrice = success.reduce((acc, curr) => acc + curr.givenPoint, 0); | ||
| const hasSuccess = success.length > 0; | ||
| const hasErrors = errors.length > 0; | ||
|
|
||
| if (hasSuccess && hasErrors) { | ||
| return ( | ||
| <div> | ||
| <p> | ||
| {success.length}마리 판매 완료, {errors.length}마리 판매 실패 | ||
| </p> | ||
| <p>총 판매 금액: {totalPrice}P</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (hasSuccess) { | ||
| return ( | ||
| <div> | ||
| <p>{success.length}마리 판매 완료</p> | ||
| <p>총 판매 금액: {totalPrice}P</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (hasErrors) { | ||
| return ( | ||
| <div> | ||
| <p>{errors.length}마리 판매 실패</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div> | ||
| <p>0마리 판매 완료, 0마리 판매 실패</p> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
컴포넌트 개선이 필요합니다
다음과 같은 개선사항들이 있습니다:
errors배열의 타입 안전성이 보장되지 않습니다- 다국어 지원이 누락되었습니다
- 가격 포맷팅이 누락되었습니다
- 중첩된 div/p 태그들의 접근성 개선이 필요합니다
다음과 같이 개선해보세요:
-const DropPetsResult = ({ success, errors }: { success: { givenPoint: number }[]; errors: unknown[] }) => {
+interface DropPetError {
+ message: string;
+ code: string;
+}
+
+const DropPetsResult = ({ success, errors }: {
+ success: { givenPoint: number }[];
+ errors: DropPetError[]
+}) => {
+ const t = useTranslations('Laboratory.property-pet-sell');
const totalPrice = success.reduce((acc, curr) => acc + curr.givenPoint, 0);
const hasSuccess = success.length > 0;
const hasErrors = errors.length > 0;
+
+ const formatPrice = (price: number) => {
+ return new Intl.NumberFormat('ko-KR').format(price);
+ };
+
+ const renderPrice = (price: number) => (
+ <p role="status" aria-label={t('total-amount')}>
+ {t('total-amount-text', { amount: formatPrice(price) })}
+ </p>
+ );
if (hasSuccess && hasErrors) {
return (
- <div>
- <p>
- {success.length}마리 판매 완료, {errors.length}마리 판매 실패
- </p>
- <p>총 판매 금액: {totalPrice}P</p>
- </div>
+ <div role="alert">
+ <p>{t('partial-success', {
+ success: success.length,
+ errors: errors.length
+ })}</p>
+ {renderPrice(totalPrice)}
+ </div>
);
}
// ... similar changes for other conditions번역 키를 추가하세요:
// messages/ko.json
{
"Laboratory": {
"property-pet-sell": {
"total-amount": "총 판매 금액",
"total-amount-text": "총 판매 금액: {amount}P",
"partial-success": "{success}마리 판매 완료, {errors}마리 판매 실패"
// ... other translations
}
}
}| <p>총 판매 금액: {totalPrice}P</p> | ||
| </div> | ||
| ), | ||
| description: <DropPetsResult success={res.success} errors={res.errors} />, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
다이얼로그 개선이 필요합니다
다이얼로그에 다음과 같은 개선사항들이 있습니다:
- 다이얼로그 제목이 하드코딩되어 있습니다
- 판매 중 로딩 상태 표시가 없습니다
- 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.
| const DropPetsResult = ({ success, errors }: { success: { givenPoint: number }[]; errors: unknown[] }) => { | ||
| const totalPrice = success.reduce((acc, curr) => acc + curr.givenPoint, 0); | ||
| const hasSuccess = success.length > 0; | ||
| const hasErrors = errors.length > 0; | ||
|
|
||
| if (hasSuccess && hasErrors) { | ||
| return ( | ||
| <div> | ||
| <p> |
There was a problem hiding this comment.
얼리 리턴을 적용하신 것은 저도 좋다고 생각해요!
하지만 순서를 바꾸면, totalPrice는 연산을 안해도 될 거 같기도 하네요 ~
읽기 좋은 순서와 연산에 대한 트레이드오프도 고려하셔서 반영해주시면 좋을 거 같은데, 의견이 궁금해요! 👍
There was a problem hiding this comment.
남겨주신 코멘트 잘 봤습니다 !
항상 연산을 먼저하고 코드에 구현하는 습관이 있었던 것 같네요 ! 말씀해주신 내용에 대해 고민해봤고, 실제로 얼리 리턴을 적용한다면 연산을 하지 않을 수 있다는 장점이 있겠네요
감사합니다. 해당 내용은 잘 반영해두었습니다.
| <p>총 판매 금액: {totalPrice}P</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (hasSuccess) { | ||
| return ( | ||
| <div> | ||
| <p>{success.length}마리 판매 완료</p> | ||
| <p>총 판매 금액: {totalPrice}P</p> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
코드를 보셔서 아시겠지만, 저희 서비스는 i18n 지원을 지향하고 있어요
해당 영역에 대한 기여도 부탁드려도 될까요? 🙏 🙏 🙏
There was a problem hiding this comment.
i18n 지원을 적용해놓았습니다. !
'Laboratory.property-pet-sell'
해당 부분에 대한 한국어, 영어 지원을 변경해놓았습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/app/[locale]/laboratory/property-pet-sell/page.tsx (1)
117-117: 🛠️ Refactor suggestion다이얼로그 제목을 현지화해주세요.
다이얼로그 제목이 하드코딩되어 있습니다.
showDialog({ - title: '펫 판매 완료', + title: t('saleComplete'), description: <DropPetsResult success={res.success} errors={res.errors} />, });
🧹 Nitpick comments (3)
apps/web/messages/en_US.json (1)
144-147: 영문 번역의 대소문자를 일관되게 수정해주세요.현재 번역:
- "sale success" -> 소문자
- "sale fail" -> 소문자
- "Total Amount" -> 대문자
모든 메시지의 첫 글자는 대문자로 통일하는 것이 좋습니다.
- "saleSuccess": "sale success", - "saleFail": "sale fail", + "saleSuccess": "Sale success", + "saleFail": "Sale fail", "totalAmount": "Total Amount"apps/web/src/app/[locale]/laboratory/property-pet-sell/page.tsx (2)
76-76: 가격 포맷팅이 필요합니다.금액을 표시할 때 천 단위 구분자를 사용하면 가독성이 향상됩니다.
+ const formatPrice = (price: number) => { + return new Intl.NumberFormat('ko-KR').format(price); + }; + const totalPrice = success.reduce((acc, curr) => acc + curr.givenPoint, 0); // ... 사용시 - {t('totalAmount')}: {totalPrice}P + {t('totalAmount')}: {formatPrice(totalPrice)}PAlso applies to: 86-87, 99-101
57-61: 접근성 개선이 필요합니다.결과를 표시하는 div에 적절한 ARIA 속성이 없습니다.
- <div> + <div role="alert" aria-live="polite">Also applies to: 67-73, 80-89, 93-102
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/messages/en_US.json(1 hunks)apps/web/messages/ko_KR.json(1 hunks)apps/web/src/app/[locale]/laboratory/property-pet-sell/page.tsx(2 hunks)
🔇 Additional comments (2)
apps/web/messages/ko_KR.json (1)
144-149: 번역이 명확하고 일관성이 있습니다!판매 상태 메시지에 대한 번역이 사용자 친화적이며 PR의 목적에 잘 부합합니다.
apps/web/src/app/[locale]/laboratory/property-pet-sell/page.tsx (1)
52-54: 🛠️ Refactor suggestion타입 안전성 개선이 필요합니다.
errors배열의 타입이unknown[]으로 되어있어 타입 안전성이 보장되지 않습니다.- const DropPetsResult = ({ success, errors }: { success: { givenPoint: number }[]; errors: unknown[] }) => { + interface DropPetError { + message: string; + code: string; + } + + const DropPetsResult = ({ success, errors }: { + success: { givenPoint: number }[]; + errors: DropPetError[] + }) => {Likely invalid or redundant comment.
|
갑자기 PR이 올라와 당황하셨을 것 같습니다. git animals를 사용하고 있던 과정에서 기여를 하고싶다는 마음에 이슈를 먼저 올려야겠다는 생각을 차마 하지 못한 것 같습니다 🥲 혹시나 추가적인 개선이 필요하다면 코멘트 남겨주세요. 감사합니다. |
| <p> | ||
| {errors.length} | ||
| {t('count')} {t('saleFail')} | ||
| </p> |
There was a problem hiding this comment.
const errorCount = error.length;
const 개 = t('count');
const 실패 = t('saleFail');
...
return <p>{errorCount}{개} {실패}</p>이런 형태는 어떻게 생각하시나요?
-
errorLength
- 현재는 단순히 배열의 길이로 판단하고 있지만, 이후 API 스펙이 변경되어 성공 객체의 특정 프로퍼티 기준으로 분기를 나눠야할 경우 대처가 쉬워진다고 생각하고 && 미약하지만 매번 프로토타입 체이닝을 하는 것보다 성능상 우위를 가질 수 있어요
-
const 개 = t('count');
- 읽기 좋은 코드에 가까워진다고도 생각이 들며, 위와 마찬가지로 매번 접근하는 것보다 성능상 우위가 있다고 생각해요
단순히 제 생각뿐이며, 의견 부탁드려요 ~ 👍
There was a problem hiding this comment.
코드 가독성면에서 고려를 하지 못했네요 ,, 성능적인 부분에 있어서는 우위가 있다는걸 처음알게 되었습니다. !
덕분에 많은 내용을 배우고 있습니다. 감사합니다.
해당 부분은 count saleSuccess saleFail totalAmount네가지로 변수 선언해서 재사용했습니다. 👍
There was a problem hiding this comment.
작업해주셔서 제가 더 감사드리죠! 👍
추가적으로 궁금한 점이, 제가 영어에 약해서 그런지 몰라도 개와 count 중 읽기 자연스러운 건 개라고 생각이 드는데요
저보다 더 많이 고민해보셨을, 작업자 분께서 영어로 네이밍하신 이유가 궁금해요!
There was a problem hiding this comment.
변수명을 한글로 선언하는게 습관이 안되어있어서 개라고 남겨주셨음에도 count를 선택했습니다. 혹시 코드컨벤션이나 이런것들을 고려했을 때 개라는 변수명을 사용하는것이 더 직관적이실까요??
There was a problem hiding this comment.
제가 읽었을 때에는 한글이 더 읽기 쉬운데, 이 부분은 작업자분께서 판단해주세요 ~
일단 어푸루부 남길게요
작업 완료하시면 댓글 부탁드려요 👍
There was a problem hiding this comment.
의도했던 작업은 모두 완료된 것 같습니다 👍
혹시, 또 다른 이슈를 발견하면 이슈에 남기고 기여를 요청드려도 괜찮을까요 ??
현재 기여를 계속 받을 계획이 있으신지에 대해 알려주신다면 감사하겠습니다.
There was a problem hiding this comment.
@hyesungoh 혹시 추가작업 필요한게 있을까요 ??
There was a problem hiding this comment.
아 제가 정신이 없어서 요즘, 이제야 봤네요 ㅎ
네 추가 기여는 너무 감사드리죠!
현재 PR에서 추가 작업이 필요해 보이는 건 없고, @sumi-0011 님이 확인해주신 후 머지 부탁드려요!
💡 기능
xx마리 판매 실패라는 문구가 등장하며 사용자에게 혼란을 제공🔎 기타
Summary by CodeRabbit
DropPetsResult컴포넌트 추가