-
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
KDT0_ParkNaYoung 프로젝트 매니지먼트 서비스 #66
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for effulgent-axolotl-ab38e8 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
디렉터리 구조 아쉽다 하셨는데 제가 보기엔 깔끔한 것 같습니다! 다양한 기능들을 넣으셨고 결과물도 정말 완벽한것 같아요 수고하셨습니다! |
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/public/js/common/membersList.js
Outdated
import { handleTooltipClick } from './tooltip' | ||
// import { handleupdateBtn } from './upadate' | ||
|
||
export function buildHTMLList(member) { |
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.
넵 여기서 시간을 많이 잡아먹었습니다😭 구조가 복잡해서 먼저 html로 만들어보고 js로 옮겼어요!
오 아예 요소를 만드는 부분도 함수로 만들면 좋을 것 같네요 좋은 의견 감사합니다!
src/public/js/common/lazy-load.js
Outdated
observer.unobserve(entry.target) | ||
} | ||
}) | ||
}, options) |
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.
어차피 나머지 값은 디폴트이니 options
=> { threshold: 0.1 }
이렇게 바꿔도 괜찮을 것 같아요! 저도 시도해본 건 아니라 확실치는 않아요ㅎㅎ..
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.
아 그렇게 해도 똑같이 동작할 것 같네요~ 리팩토링할 때 시도해보겠습니다 😄
// 스토리지에 업로드 | ||
export async function uploadStorage(file) { | ||
const fileName = file.name | ||
const id = Date.now() / fileName.length |
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.
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.
id는 고유값을 생성하는 부분이에요! storage에 파일을 업로드할 때 이름에 고유값을 넣어서 최대한 중복이름을 피하고자 했습니다.
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.
fileName
을 해싱해서 Date.now()
와 조합했으면 조금 더 unique한 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.
모듈화 관련해서 정말 많은 고민을 하셨던 걸로 아는데 그 고민의 흔적이 가득 녹아있는 코드였습니다..! 엄청 깔끔하게 정리해두셔서 감탄하면서 봤습니다 :) 심지어 사이트도 예뻐요ㅎㅎ 최고세요!!!!!!!!!!🤩
+) 아주 사소한 문제 하나 발견했습니당. 반응형 가로 391px~753px정도를 지날때 멤버리스트에서 글씨 및 아이콘이 조금 빠져나가게 보여요
imageUrlInfo.revokeImageUrl() | ||
} | ||
} catch (error) { | ||
console.error('Error during form submission: ', error) |
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.
Add member버튼 통해서 업로드 할 때 이런 오류가 발생합니다!
업로드는 정상적으로 되지만 한번 확인해보시면 좋을 것 같습니다.
members.controller.js:73 Error during form submission: ReferenceError: imageUrlInfo is not defined
at HTMLFormElement. (members.controller.js:69:1)
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/public/js/app.js
Outdated
if (e.target.classList.contains('members__row')) { | ||
const memberId = e.target.getAttribute('data-id') | ||
const newUrl = `/members/${memberId}` | ||
navigateTo(newUrl) |
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.
이런 식으로 해쉬를 사용하는 방법도 있었군요:D 배워갑니다 ㅎㅎ
다만 글씨/사진 근처 부분을 클릭했을 때는 이동하지 않고, 커서가 pointer로 바뀌지 않아 동작을 확인하는 게 쉽지 않았습니다..!
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.
이동이 제대로 안되는 부분은 어떻게 해결해야할 지 저도 고민인 부분입니다 😭.. 리팩토링하면서 왜 이동이 안되는지 원인을 찾아봐야겠어요!
커서를 pointer로 바꾸는게 훨씬 접근성이 좋을 것 같네요 좋은 의견 감사합니다!!
폴더 구조 아쉽다고 하셨는데 폴도 구조도, 코드도 너무 잘 정리되어있고 깔끔해서 가독성이 높았습니다! 개인적으로 팀 영역에 마우스 각 팀의 멤버들 뜨는 거 좋았어요!! 프로필 상세 보기 아이콘이 크기가 작고, 리스트의 마우스 오버 효과로 인해서 상세보기 아이콘 위치도 달라져서 곧바로 클릭하기가 조금 불편했어요..! 아이콘 크기가 작은만큼 수정/삭제 버튼 아이콘이 리스트에 바로 노출되면 사용성이 더 좋을 것 같아요! 모바일에서 프로필 상세보기 아이콘 클릭할 때 '박지민'분만 레이아웃이 가로형으로 변경됩니다! |
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.
고생하셨습니다!
짧은 시간에 구현하실 수 있을 분량이 아니었을 것 같은데, 잠은 주무시면서 하시는 거죠?
dom 생성 부분같은 것과 프로젝트 구조만 조금 다듬으면 아주 좋아질 것 같습니다!
[[redirects]] | ||
from = "/*" | ||
to = "/index.html" | ||
status = 200 |
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.
_redirects
파일이랑 netlify.toml
둘 다 사용해야 작동하는 건가요??
src/index.html
Outdated
@@ -0,0 +1,14 @@ | |||
<!DOCTYPE html> | |||
<html lang="en"> |
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.
en
-> ko
<title>Main</title> | ||
</head> | ||
<body> | ||
<div id="app"></div> |
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.
html이 극단적으로 다이어트했네요
src/public/asset/styles/common.scss
Outdated
@media (min-width: 1200px) { | ||
.container-xxl, | ||
.container-xl, | ||
.container-lg, | ||
.container-md, | ||
.container-sm, | ||
.container { | ||
max-width: 960px; | ||
} | ||
} |
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/public/asset/styles/common.css
Outdated
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.
커밋되지 않아야 하는 파일이 같이 커밋된 것 같네요...!
rmBtn.style.display = showRmBtn ? 'block' : 'none' | ||
cgBtn.style.display = showCgBtn ? 'block' : 'none' | ||
saveCgBtn.style.display = showSaveCgBtn ? 'block' : 'none' |
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.
hidden
attribute을 toggle하거나, css로 조절해보셔도 좋을 것 같습니다.
} | ||
|
||
&__image { | ||
flex-grow: 0 !important; |
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.
!import
의 사용은 최대한 지양해주시는 게 좋습니다.
const getAllTeams = () => { | ||
return new Promise((resolve, reject) => { |
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.
이런 케이스엔 return을 생략해도 되지 않을까요?
const getUsersByTeam = (teamName) => { | ||
return new Promise((resolve, reject) => { | ||
try { | ||
const q = query(collection(db, 'members'), where('team', '==', teamName), orderBy('name')) |
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.
q
보단 조금 더 성의있는 이름을 지어주시면 좋을 것 같습니다.
const members = [] | ||
querySnapshot.forEach((doc) => { | ||
members.push(doc.data()) | ||
}) |
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.
map을 사용하면 좀 더 깔끔하고 직관적으로 정리할 수 있지 않을까요?
프로젝트 매니지먼트 서비스
🚀 프로젝트 소개
팀 프로젝트 진행 시 멤버를 등록하고 팀 구성을 도와주는 서비스를 입니다 😁
🚀 프로젝트 배포
https://deploy-preview-66--effulgent-axolotl-ab38e8.netlify.app/
🚀 과제 요구사항
✅ 필수 요구사항
✅ 선택 요구사항
🚀 프로젝트 내용
✓ floating animation
✓ lazy-loading
✓ 멤버 등록순 정렬
✓ lazy-loading
✓ 모든 폼을 채워야 등록 가능
✓ 팀 이름순 정렬
✓ hover animation
✓ lazy-loading
✓ unsplash random 이미지 사용
🚀 유저 플로우
interaction 기준으로 작성했습니다
✍️ 아쉬운 점