Skip to content

Kwon ye kyeong#1

Open
KwonYeKyeong wants to merge 7 commits intoKwonYeKyeong-Devfrom
KwonYeKyeong
Open

Kwon ye kyeong#1
KwonYeKyeong wants to merge 7 commits intoKwonYeKyeong-Devfrom
KwonYeKyeong

Conversation

@KwonYeKyeong
Copy link
Collaborator

@KwonYeKyeong KwonYeKyeong commented Aug 3, 2020

  • 등록 날짜 표시
  • 카드 삭제 버튼 구현 및 BE 서버에 삭제 요청
  • 담당자 추가
  • 우선순위 추가 및 우선순위를 기준으로 오름차순 정렬

Copy link
Collaborator

@so3500 so3500 left a comment

Choose a reason for hiding this comment

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

.idea 폴더 이하 파일은 개개인의 IDE 설정 파일이므로 추가하지 않아도 돼요~ (삭제 부탁드려요)
(@raccoonback 님이 .gitignore 로 idea 폴더는 무시하도록 설정했을 텐데 이상하네요 🤔 )

@raccoonback
Copy link
Collaborator

@so3500
.gitignore 파일이 static 디렉토리 안에 있어서 .idea 관련 파일들에 대해서 적용이 안된거 같아요..

해당 부분은 슬랙에 남겨놓을게요~

Copy link
Collaborator

@raccoonback raccoonback left a comment

Choose a reason for hiding this comment

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

기본 코드 구조가 복잡해서 힘들었을텐데...

요구사항에 맞춰서 잘 구현해준 거 같아~ 👍

몇 가지 리뷰남겼는데, 시간있으면 참고해서 수정해봐도 좋을 거 같아~

return a.priority-b.priority;
});
cards.done.sort(function (a, b) {
return a.priority-b.priority;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sort() 함수 잘 사용했네 굿굿 👍 👍 👍

몇가지 추가로 살펴보면
Tasktodo 에서 done으로 이동한 경우에는 todo를 새로 정렬할 필요없고,
반대로 done 에서 todo로 이동한 경우에도 done을 새로 정렬할 필요없거든

그래서, Task가 이동한 Card 상태를 의미하는 changedStatus(todo or done) 를 이용해서,
변경된 Card 대해서만 정렬를 수행해주면 불필요한 작업은 줄일 수 있어~

Comment on lines +54 to +56
let index = todo.findIndex((value) => {
return value.id === id;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 코드가 65번 라인의 index = done.findIndex((value) => { return value.id === id;}); 코드하고 비슷한 형식으로 중복되는 것을 볼 수 있는데,

index = done.findIndex((value) => {
return value.id === id;
});

remove() 함수 인자로 status 값까지 받아온다면, 아래처럼 좀더 일반화된 방식으로 중복 코드를 제거할 수 있을 거 같아~

function remove(id, status) {
       ...
       const index = cards[originStatus].findIndex((task) => task.id === id);
       ...
}

Comment on lines +58 to +62
todo.splice(index, 1);
const isSuccesses = deleteCard(id);
if (!isSuccesses) {
alert('NotFound(todo)');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 부분도 68~72 라인하고 중복되는 형식인거 같아

done.splice(index, 1);
const isSuccesses = deleteCard(id);
if (!isSuccesses) {
alert('NotFound(done)');
}

일반화하기 앞서서, 먼저 중복 코드를 별도의 함수로 분리해보는 것도 좋을거 같아~

function Enrollment({history}) {
const [title, setTitle] = useState('');
const [assignee, setAssignee] = useState('');
const [priority, setPriority] = useState('2');
Copy link
Collaborator

Choose a reason for hiding this comment

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

초기화 굿굿 👍 👍

event.stopPropagation();
setTitle('');
setAssignee('');
document.getElementById("priority").value = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

리액트에서는 브라우저에서 지원하는 DOM에 직접 접근하는 방식보다 Ref 라는 방식
을 통해서 Element에 접근하는 것을 권장하고 있어.

아래 링크를 통해서도 자세한 이유를 확인할 수 있어
Ref가 document.getElementById 보다 나은 이유

(확실하진 않지만, DOM에 직접 접근하게 되면 React가 제공하는 가상 DOM의 장점을 잃게 되지 않을까..?)

Hooks API로는 useRef()를 사용할 수 있어~

const priorityEl = useRef(null);

const onClearClick = (event) => {
  ...
  priorityEl.current.value = 2;
  ....
}

return (
   ...
   <select id='priority' onChange={onChangePriority}" ref={el}>
      ....
    </select>
   ....
);

두 번째로, select Element에 접근하는 방법보다는 useState()로 priority 상태를 관리하면 어떨까?

버튼 클릭 이벤트 발생시에는 setXXX() 수정 메서드로 우선 순위 상태를 변경하는 방식도 해보면 좋을거 같아~

const [priority, setPriority] = useState(2);
...
const onClearClick = (event) => {
  ...
  setPriority(2);
  ....
}
...

return (
   ...
   <select id='priority' onChange={onChangePriority}" value={priority}>
      ....
    </select>
    ....
);

우선순위
<select id='priority' onChange={onChangePriority}>
<option value="1">1순위</option>
<option selected value="2">2순위</option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

selected 굿굿 💯

Comment on lines +71 to +73
<option value="1">1순위</option>
<option selected value="2">2순위</option>
<option value="3">3순위</option>
Copy link
Collaborator

@raccoonback raccoonback Aug 4, 2020

Choose a reason for hiding this comment

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

이런거 map() 함수 적용해봐도 좋을듯

Ex)

[1, 2, 3].map( ... );

'Content-Type': 'application/json; utf-8',
},
});
return response.status === 201;
Copy link
Collaborator

Choose a reason for hiding this comment

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

삭제 성공한 경우, status code => 204(No Content)
https://docs.google.com/document/d/18hPE1bP4o4xP9xIAbHOL5-baelotFG9_zz8uJUPPVzw/edit

isTodo(subject) && <Button className='task-left-btn' onClick={onClick} value='>'/>
}
{title}
{title} {String(priority)+'순위'} <br/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

String 타입으로 변경하지 않아도, '순위' 문자열이 추가되면서 string 타입으로 변경될 거 같아~

Copy link
Collaborator

@so3500 so3500 left a comment

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

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants