-
Notifications
You must be signed in to change notification settings - Fork 0
PR:review api 생성했습니다 #69
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
base: main
Are you sure you want to change the base?
Conversation
ijimlnosk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 params 수정을 해주셔야 될것같습니다!
src/libs/axios/base/review.js
Outdated
| images, | ||
| }) => { | ||
| const response = await axiosInstance.post( | ||
| `/review?payList_idx=${payList_idx}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주소에 들어가는 params는
params { } , 이렇게 작성하기로 했습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/libs/axios/base/review.js
Outdated
| } | ||
|
|
||
| --- 구매 내역 -- | ||
| PayList: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 주석인가요????
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response에 대해 적어놓은 것 같은데 없어도 될 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰남겼습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 전반적으로 함수명과 주소 부분 수정이 필요할 것 같습니다.
(ByIndex => ByPayListIdx,ByReviewIdx등등) - 리뷰 상세의 경우
getReviewByIndex로 작성하였는데getReviewGetByReviewIdx가 컨벤션에 맞지 않을까요? - 진솔님도 언급하신 부분인데 params 부분 수정해주시고 parameter의 스네이크 케이스는 컨벤션에 맞춰서 변경해주세요 :)
- response에 대해서도 전부 주석으로 달아주신 거 같은데 그 부분은 없어도 되지않을까 싶네요.
src/libs/axios/base/review.js
Outdated
| - 요청결과 200("success") || 400("failure") | ||
| */ | ||
| export const postReviewByIndex = async ({ | ||
| payList_idx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- snake case는 컨벤션에 맞지 않아요. 수정해주세요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 스네이크 케이스는 가져가는게 좋지않을까 합니다....?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/libs/axios/base/review.js
Outdated
| } | ||
|
|
||
| --- 구매 내역 -- | ||
| PayList: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response에 대해 적어놓은 것 같은데 없어도 될 것 같아요.
| { title, content, ondo, images } | ||
| ); | ||
|
|
||
| return response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- 이 부분만 return값이 response인 이유가 있을까요? 다른 부분은 다 response.data인 것 같은데..
src/libs/axios/base/review.js
Outdated
| * @description 리뷰들의 목록을 보여주는 api | ||
| */ | ||
|
|
||
| export const getReviewList = async ({ page }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- page가 query string이라면 함수명을 컨벤션에 맞춰 getReviewByPage로 해야하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다. 컨벤션이 안지켜진것같네요
src/libs/axios/base/review.js
Outdated
| /** | ||
| * 3.리뷰 상세 | ||
| * @function | ||
| * @parameter review_idx: number - 조회할 리뷰의 idx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- reviewIdx로 수정해야 할 것 같습니다.
| { params: { review_idx: reviewIdx } }, | ||
| { title, content, ondo, img_url, main_url, images } | ||
| ); | ||
| return response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- 이 부분도 return값이 response네요. 하나로 통일시키는게 좋을 것 같습니다.
| const response = await axiosInstance.delete("/review", { | ||
| params: { review_idx: reviewIdx }, | ||
| }); | ||
| return response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- 이 부분도 return값이 response네요. 하나로 통일시키는게 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api요청에따라 데이터 값만 필요한 조회(목록,상세)부분은 response.data로 해주었고 나머지 3요청들은 header 및데이터 외의 값이 필요한게있다고 해서 response로 주었습니다
src/libs/axios/base/review.js
Outdated
| * - 요청결과 200("success") || 400("failure") | ||
| */ | ||
|
|
||
| export const patchReviewByReviewIdx = async ({ reviewIdx }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- parameter로 reviewIdx만 들어가나요? body부분이 빠진 것 같습니다.
src/libs/axios/base/review.js
Outdated
| const response = await axiosInstance.patch( | ||
| "/review", | ||
| { params: { review_idx: reviewIdx } }, | ||
| { title, content, ondo, img_url, main_url, images } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- snake case가 남아있네요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hayoung78
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR 올리기전에 한번 더 확인 해주십쇼~~ (저도 그렇긴하지만 ) 뭔가 급하게 올린것같은? 느낌 있습니다요 :) 굳
src/libs/axios/base/review.js
Outdated
| * @description 리뷰들의 목록을 보여주는 api | ||
| */ | ||
|
|
||
| export const getReviewList = async ({ page }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다. 컨벤션이 안지켜진것같네요
src/libs/axios/base/review.js
Outdated
| * @description 특정한 하나의 review를 나타내는 API | ||
| * - 요청한 특정 리뷰가 상세히 조회됩니다 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 두줄이상이면
@description
- 뭐뭐뭐
- 뭐뭐뭐
이런식으로 작성해주세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 남겼습니다
src/libs/axios/base/review.js
Outdated
| ex) 전체 온도 70도 / 판매갯수 2 => 35도 | ||
| - 요청결과 200("success") || 400("failure") | ||
| */ | ||
| export const postReviewByPayListIndex = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
Index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정부탁바람
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/libs/axios/base/review.js
Outdated
| ex) 전체 온도 70도 / 판매갯수 2 => 35도 | ||
| - 요청결과 200("success") || 400("failure") | ||
| */ | ||
| export const postReviewByPayListIndex = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정부탁바람
…into feat/api-review
ijimlnosk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했습니다
TransparentDeveloper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오... 굳 진짜 깔끔한데요??
src/libs/axios/base/review.js
Outdated
| img_url: imgUrl, | ||
| main_url: mainUrl, | ||
| images, | ||
| }) => { | ||
| const response = await axiosInstance.patch( | ||
| "/review", | ||
| { params: { review_idx: reviewIdx } }, | ||
| { title, content, ondo, img_url: imgUrl, main_url: mainUrl, images } | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Level
- N : "🔥 이대로 Merge 하면 안돼요~!"
- M : "🥹 고치면 분명 나아질 게 분명합니다.."
- S : "🤷 수정하면 좋지 않을까요?"
Description
- 잘 이해가 안돼서 설명을 요청드려요.~
- 다음과 같이 작성하지 않은 이유가 있을까요??
export const patchReviewByReviewIdx = async ({
...,
imgUrl, // 여기 수정
mainUrl, // 여기 수정
images,
}) => {
const response = await axiosInstance.patch(
"/review",
{ params: { review_idx: reviewIdx } },
{ title, content, ondo, img_url: imgUrl, main_url: mainUrl, images }
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…into feat/api-review
🎯 작업 내용
✅ 체크 리스트
😭 꼭 리뷰 남겨주세요.!!
👍 문제가 없다고 생각된다면...
🙏 코드 수정 요청을 원하신다면...
2단계까지 진행해주세요.where/how/why3하 원칙(?) 에 따라, 구체적인 요청사항을 작성해주세요.