-
Notifications
You must be signed in to change notification settings - Fork 0
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
KDT5_Team_6 과제 제출 #2
base: main
Are you sure you want to change the base?
Conversation
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 관련 타입 설정이나 예외처리 부분 인스턴스화, 네이밍 등 조금 더
디테일하게 작업 하면 좋을 것 같습니다.
파일명이 index로 되어있는 경우가 많아 구분이 어려운 것 같습니다.
굳이 폴더를 만들어 깊게 파일을 넣을 필요 없을 것 같습니다.
폴더는 그룹핑이 필요할 때 사용하시면 좋을 것 같아요.
불필요한 요청이나 렌더링이 발생하게 되는 부분이 있습니다.
이런 부분은 기간적으로 짧아서 어쩔 수 없는 부분이라 생각합니다.
시간이 되면 몇군데 최적화를 해보시면 좋을 것 같아요.
질 만드셨습니다! 고생하셨습니다!
for (let product of products) { | ||
await api({ | ||
method: 'DELETE', | ||
url: `/api/products/${product.id}`, | ||
}) | ||
} |
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.
반복문 안에서 비동기 처리가 많이 이루어 지는 것은 지양하는 것이 좋을 것 같아요.
특정 개수 만큼 Promise.all 등으로 처리하는 등 개선이 필요해 보입니다.
const api = axios.create({ | ||
baseURL: process.env.BASE_URL, | ||
headers: { | ||
apiKey: process.env.API_KEY, | ||
username: process.env.USERNAME, | ||
masterKey: process.env.MASTER_KEY, | ||
}, | ||
}) |
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.
반복적으로 사용되어 따로 관리하여 재사용 하는 것도 좋을 것 같습니다.
await axios(' https://asia-northeast3-heropy-api.cloudfunctions.net/api/products', { | ||
method: 'POST', | ||
data: JSON.stringify(productRequestBody), | ||
headers: { | ||
'Content-Type': 'application/json', | ||
masterKey: true, | ||
apikey: 'KDT5_nREmPe9B', | ||
username: 'KDT5_Team6', | ||
}, | ||
}) |
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.
이 부분 또한 메모리 상에 products list를 잡아 놓고
직렬 요청이 아닌 요청 수를 늘려 병렬로 요청을 보내는 것이 좋아보입니다.
url: `/api/handler?id=${username}&password=${password}`, | ||
method: 'GET', | ||
}) | ||
return data.isAdmin |
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.
apis/admin 내에 모든 함수의 return 타입이 any 인 것 같습니다.
ts를 쓰는 만큼 return 의 타입을 알 수 있으면 좋지 않을까요?
if (id && password) { | ||
authenticateAdmin(id, password).then((res) => { | ||
setIsAdmin(res) | ||
}) | ||
} |
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.
localStorage.getItem 에서 id나 password를 못 가져오게 되면
authenticateAdmin이 실행되지 않아 isAdim이 최신화 되지 않을 것 같습니다.
<input | ||
id="tel" | ||
className={`${styles.inputTag} ${styles.tel}`} | ||
type="text" | ||
name="tel" | ||
value={phoneNum} | ||
onChange={inputHandlePhone} | ||
placeholder="전화번호를 입력해주세요." | ||
required | ||
/> | ||
</form> |
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.
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_input_type_tel
inputHandlePhone 의 처리를 조금 더 간소화 시킬 수 있을 것 같습니다.
useEffect(() => { | ||
connectAccountData({ | ||
bankCode: code, // 연결할 은행 코드 (필수!) | ||
accountNumber: accountNum.replace(/-/g, ''), // 연결할 계좌번호 (필수!) |
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.
replaceAll도 좋을 것 같습니다.
const paymentPathRegex = /^\/payment\/[^/]+$/ | ||
const agreementPathRegex = /^\/payment\/[^/]+\/agreement$/ | ||
const checkInfoPathRegex = /^\/payment\/[^/]+\/checkInfo$/ | ||
const payMethodPathRegex = /^\/payment\/[^/]+\/payMethod$/ | ||
const orderCompletePathRegex = /^\/payment\/[^/]+\/orderComplete$/ | ||
|
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.
컴포넌트 외부에 있어도 좋을 것 같습니다.
const navigation = useNavigate() | ||
const query = useRecoilValue(searchQueryState) | ||
|
||
function selectOption(e: React.ChangeEvent<HTMLSelectElement>) { |
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.
filter의 경우 항상 같은 데이터 list 를 부르기 때문에
재요청 없이 sort만 해주면 되지 싶습니다.
// * 검색 페이지내에서 URL이 변경될 때 검색어 recoil에 저장된 search 파라메터로 초기화합니다. | ||
useEffect(() => { | ||
setSearch(query.search ? query.search : '') | ||
}, [query]) |
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.
동작이 좋지 않을 것 같습니다.
특정 값을 입력하고 검색 후 값을 추가로 입력 후
검색전 filter(sort를 오름차순 내림 차순등으로 변경하면 값이 없어집니다.)
SumTendo 게임판매 웹서비스
➡️배포 - 숨텐도
➡️배포 - 숨텐도 관리자
Description
Stack
Feature
Main
Access
User
Payment
Admin
Team
Files
자료구조