Skip to content
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_Lee_Yeon_Su 직원 관리 서비스 #62

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Conversation

suehub
Copy link
Member

@suehub suehub commented Aug 18, 2023

✨ 직원 관리 서비스

📍 프로젝트 소개


✔ 구현 내용

  • 로딩 애니매이션, 직원 리스트

  • 직원 등록

  • 직원 상세

  • 직원 수정

  • 직원 이름과 이메일로 검색

  • 직원 삭제

  • 여러 직원 삭제

  • 반응형 구현 (모바일)
    모바일반응형

    • checked 직원 없이 삭제 버튼 클릭 시 alert

📍 유저 플로우

유저플로우 drawio


📌 회고

  • js로 과제를 구현하면서 dom 조작에 익숙해질 수 있었다.
  • 파이어베이스를 처음 사용해보는데 연동하는 것에만 며칠이 걸린 것 같다.
    영상을 따라서 연동에 성공했는데 v8로 연동해서 v9부터 추가된 유용한 메소드들를 사용하지 못해 아쉬웠다.

@netlify
Copy link

netlify bot commented Aug 18, 2023

Deploy Preview for my-employee-management ready!

Name Link
🔨 Latest commit 787995a
🔍 Latest deploy log https://app.netlify.com/sites/my-employee-management/deploys/6501dd344571a100075da5ee
😎 Deploy Preview https://deploy-preview-62--my-employee-management.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@humnyenye
Copy link

저로 리스트 구조를 짤 때 table태그로 할지 flex나 grid로 할지 고민 많이 했는데 table로 짠 구조를 볼 수 있어서 좋았습니다.
employee-detail__modal의 height값이 고정되어 있어서 하단의 버튼들이 박스 바깥으로 튀어나가 보였습니다. 내부 요소들에 의해 자연스럽게 height가 조절될 수 있도록 값을 빼주거나 아니면 더 높게 잡아야 할 것 같아요. 그리고 팝업 자체도 left를 중앙 정렬하는게 깔끔해 보이지 않을까 합니다.

@LeHiHo
Copy link

LeHiHo commented Aug 23, 2023

검색 기능 구현하실때 모두삭제하고 검색어와 일치하는거 불러오는 아이디어 배워갑니다.
실시간+일부분만 일치해도 검색되는 부분도 좋았어요
중간 단어가 일치해도 검색되면 좋을것 같습니다.
예를 들어 강나라를 검색할때 -> '강나'로 검색⭕️ / '나라'로 검색❌
사소한 ux공유

  • 체크박스 누르려고 마우스올리면 체크박스가 도망가요~
  • 프로필 수정페이지 들어갈때 어디를 눌러야할지 고민했어요

Copy link

@noSPkeepgoing noSPkeepgoing left a comment

Choose a reason for hiding this comment

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

저랑 비슷하게 생각하고 구현하신 부분이 곳곳에 보여서 재밌게 봤어요! 고생하셨습니다✨👏


// 직원 상세 모달
function setModal(employee) {
console.log(employee);

Choose a reason for hiding this comment

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

테스트한 흔적들이 이곳저곳 조금 보이는데, 콘솔로그 지우고 커밋하는 연습하면 좋을 듯 합니당!

Comment on lines +14 to +16
closeIcon.addEventListener("click", () => {
closeIcon.parentNode.parentNode.style.display = "none";
});

Choose a reason for hiding this comment

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

추가로 모달 컨테이너를 제외한 배경을 클릭하면 모달을 제거하는 로직이 있어도 좋을 거 같아요!

.gitignore Outdated
.env

# firebase file
keys.js

Choose a reason for hiding this comment

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

gitignore파일이 정상적으로 동작하지 않는 거 같아요. keys파일 노출되어 있어요!

Copy link
Member

Choose a reason for hiding this comment

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

git이 track을 시작한 시점 이후에 .gitignore에 추가돼서 그렇습니다.

  • 0a5794a - keys.js가 추가된 시점
  • db17ea5 - .gitignore가 업데이트된 시점

Removing sensitive data from a repository를 참고하셔서 파일을 지우시거나, 간단하게 keys.js를 삭제하는 커밋을 남기고 key를 모두 만료시키고 새로 발급하시는 게 좋을 것 같네요.

function createEmployee() {
const employeeForm = document.querySelector("#employee__form");

employeeForm.addEventListener("submit", (e) => {

Choose a reason for hiding this comment

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

정보가 비어있을 때 발생하는 에러 핸들링이 있으면 좋을 거 같아요!
저같은 경우에는 alert창을 띄워서 사용자에게 모든 정보를 입력하게 요구하도록 했답니다🤔

<label>이메일</label>
<input
class="detail__email"
type="email"

Choose a reason for hiding this comment

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

복잡한 정규식만 생각했었는데..! type 활용하는 방법 리마인드하고 가요✨

Comment on lines +6 to +10
setTimeout(() => {
animation.remove();
document.querySelector("body").style.background = "none";
main.style.display = "block";
}, 1500);

Choose a reason for hiding this comment

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

index.html파일이 불러올 때마다 실제 로딩시간과 상관없이 setTimeout함수를 사용해서 구현하신 이유가 있을까요?!
애니메이션 동작 시간보다 로딩시간이 짧아서 애니메이션을 모두 보여주기위해..? 라고 나름의 추측을 해봤는데 연수님의 생각이 궁금해요!

Copy link
Member Author

Choose a reason for hiding this comment

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

선파님 말씀처럼 애니메이션을 모두 보여주기 위해 setTimeout으로 작성한 것인데, 생각해보니 직원을 등록하고 삭제할 때도 로딩이 되니 로딩 시간만큼만 보여주면 될 것 같군요..! 리팩토링 때 참고하겠습니다!!

Copy link
Member

Choose a reason for hiding this comment

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

디자인 신경 많이 쓰시는 퍼블리셔 분들은 이렇게 로더가 출력되는 시간을 강제로 지정해두기도 하시더라고요.
로더가 오랫동안 출력됐는데도 화면이 다 그려지지 않은 불상사를 막기 위해서, 데이터 조회 완료 && 진입으로 부터 1.5초 지남 두 조건을 모두 만족할 때 없애는 것도 방법일 것 같네요.

아니면 유튜브처럼 스켈레톤을 사용하시면 좀 더 UI적 완성도가 높아지긴 하지만, 구현이 좀 귀찮다는 단점이 있습니다...

물론 이런 고민 할 필요 없을만큼 화면이 빨리빨리 뜨는 게 제일 좋을것 같긴 합니다...!

@kyungkyuBae
Copy link

매번 연수님 코드 볼때마다 느끼는건데 파일분리, 기능마다 함수 정리하시는거 보고 대단한것같아요 봤을때 정리가 잘되어있어서 함수명만 보더라도 어떤 기능하는지 알 수 있을거같고 그 단위단위 잘 나눠놓으신것같아서 가독성이 매우 좋은거같아요 저도 그런 코드방식으로 지향하고 싶었는데 매번 어려움을 겪었는데 잘 나누시는것 같아요

Copy link
Member

@marshallku marshallku left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!
디자인도 어느정도 규격화되어있어 보기도 편했습니다.
비동기 작업에 대한 관리, 반복되는 작업의 공통화, 안전한 참조형 변수 관리까지 되면 아주 좋을 것 같습니다!

index.html Outdated Show resolved Hide resolved
.gitignore Outdated
.env

# firebase file
keys.js
Copy link
Member

Choose a reason for hiding this comment

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

git이 track을 시작한 시점 이후에 .gitignore에 추가돼서 그렇습니다.

  • 0a5794a - keys.js가 추가된 시점
  • db17ea5 - .gitignore가 업데이트된 시점

Removing sensitive data from a repository를 참고하셔서 파일을 지우시거나, 간단하게 keys.js를 삭제하는 커밋을 남기고 key를 모두 만료시키고 새로 발급하시는 게 좋을 것 같네요.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
src/css/animation.css Outdated Show resolved Hide resolved
src/js/firebaseData.js Outdated Show resolved Hide resolved
src/js/main.js Outdated Show resolved Hide resolved
checkbox.checked = true;

// 선택된 모든 직원의 이메일 배열에 삽입
let email = tr.children[3].innerHTML;
Copy link
Member

Choose a reason for hiding this comment

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

querySelector 등을 사용해서 조금 더 명시적으로 접근해보는 게 어떨까요?

src/js/main.js Outdated Show resolved Hide resolved
src/css/modal.css Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants