Skip to content

Conversation

@keeprok
Copy link
Contributor

@keeprok keeprok commented Feb 28, 2024

🎯 작업 내용

  • 리뷰 등록
  • 리뷰 목록 조회
  • 리뷰 상세
  • 리뷰 수정
  • 리뷰 삭제

*함수컨벤션
요청종류 / 요청주소 / 정보 / By parameter



✅ 체크 리스트

  • Main 브랜치 Pull 받기
  • [x]PR 제목 (컨벤션에 따랐는지) 확인
  • Label 확인
  • Assignees 설정 확인
  • Reviewers 설정 확인


😭 꼭 리뷰 남겨주세요.!!

👍 문제가 없다고 생각된다면...

  1. "FileChanged" Tab 으로 이동합니다.
  2. "Review Changes" Button 클릭합니다.
  3. Comment 입력 (*Optional) 후, "Approve" 선택하고 "Submit Review" 버튼을 클릭합니다.

🙏 코드 수정 요청을 원하신다면...

  1. 아래의 템플릿을 복사 하여, 위의 2단계 까지 진행해주세요.
# Request Level
  - [ ] N : "🔥 이대로 Merge 하면 안돼요~!"
  - [ ] M : "🥹 고치면 분명 나아질 게 분명합니다.."
  - [ ] S : "🤷 수정하면 좋지 않을까요?"

# Description

  1. Comment 에 템플릿을 붙혀넣고, where / how / why 3하 원칙(?) 에 따라, 구체적인 요청사항을 작성해주세요.
  2. "Request changes" 선택하고 "Submit Review" 버튼을 클릭합니다.


@keeprok keeprok added the feat 추가한 (기능/문제풀이) 간략히 설명 label Feb 28, 2024
@keeprok keeprok self-assigned this Feb 28, 2024
Copy link
Contributor

@ijimlnosk ijimlnosk left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 params 수정을 해주셔야 될것같습니다!

images,
}) => {
const response = await axiosInstance.post(
`/review?payList_idx=${payList_idx}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

주소에 들어가는 params는
params { } , 이렇게 작성하기로 했습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

--- 구매 내역 --
PayList: {
Copy link
Contributor

Choose a reason for hiding this comment

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

어떤 주석인가요????

Copy link
Contributor

Choose a reason for hiding this comment

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

response에 대해 적어놓은 것 같은데 없어도 될 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

리뷰남겼습니다

Copy link
Contributor

@jjjiyoung0130 jjjiyoung0130 left a 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에 대해서도 전부 주석으로 달아주신 거 같은데 그 부분은 없어도 되지않을까 싶네요.

- 요청결과 200("success") || 400("failure")
*/
export const postReviewByIndex = async ({
payList_idx,
Copy link
Contributor

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는 컨벤션에 맞지 않아요. 수정해주세요 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

개인적으로 스네이크 케이스는 가져가는게 좋지않을까 합니다....?

Copy link
Contributor Author

@keeprok keeprok Feb 29, 2024

Choose a reason for hiding this comment

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

}

--- 구매 내역 --
PayList: {
Copy link
Contributor

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;
Copy link
Contributor

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인 것 같은데..

* @description 리뷰들의 목록을 보여주는 api
*/

export const getReviewList = async ({ page }) => {
Copy link
Contributor

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로 해야하지 않을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

동의합니다. 컨벤션이 안지켜진것같네요

/**
* 3.리뷰 상세
* @function
* @parameter review_idx: number - 조회할 리뷰의 idx
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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네요. 하나로 통일시키는게 좋을 것 같습니다.

Copy link
Contributor Author

@keeprok keeprok Mar 1, 2024

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로 주었습니다

* - 요청결과 200("success") || 400("failure")
*/

export const patchReviewByReviewIdx = async ({ reviewIdx }) => {
Copy link
Contributor

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부분이 빠진 것 같습니다.

const response = await axiosInstance.patch(
"/review",
{ params: { review_idx: reviewIdx } },
{ title, content, ondo, img_url, main_url, images }
Copy link
Contributor

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가 남아있네요 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@hayoung78 hayoung78 left a comment

Choose a reason for hiding this comment

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

PR 올리기전에 한번 더 확인 해주십쇼~~ (저도 그렇긴하지만 ) 뭔가 급하게 올린것같은? 느낌 있습니다요 :) 굳

* @description 리뷰들의 목록을 보여주는 api
*/

export const getReviewList = async ({ page }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

동의합니다. 컨벤션이 안지켜진것같네요

Comment on lines 49 to 50
* @description 특정한 하나의 review를 나타내는 API
* - 요청한 특정 리뷰가 상세히 조회됩니다
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 두줄이상이면

@description 
- 뭐뭐뭐
- 뭐뭐뭐

이런식으로 작성해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

리뷰 남겼습니다

ex) 전체 온도 70도 / 판매갯수 2 => 35도
- 요청결과 200("success") || 400("failure")
*/
export const postReviewByPayListIndex = async ({
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

수정부탁바람

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hayoung78 hayoung78 self-requested a review March 1, 2024 05:07
ex) 전체 온도 70도 / 판매갯수 2 => 35도
- 요청결과 200("success") || 400("failure")
*/
export const postReviewByPayListIndex = async ({
Copy link
Contributor

Choose a reason for hiding this comment

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

수정부탁바람

Copy link
Contributor

@ijimlnosk ijimlnosk left a comment

Choose a reason for hiding this comment

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

확인했습니다

Copy link
Contributor

@TransparentDeveloper TransparentDeveloper 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 82 to 90
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 }
);
Copy link
Contributor

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 }
	);

Copy link
Contributor Author

@keeprok keeprok Mar 3, 2024

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat 추가한 (기능/문제풀이) 간략히 설명

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants