Skip to content

Conversation

@0andrich
Copy link
Collaborator

@0andrich 0andrich commented Aug 2, 2024

제대로 올라갔나??

Copy link
Collaborator

@cryingdryice cryingdryice 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 22 to 28
<div style={{
width: "100%",
textAlign: "center",
borderBottom: "1px solid #aaa",
lineHeight: "0.1em",
}}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

div 태그의 style속성에서 스타일을 지정하기 보다
class를 선언해 css 파일에 분리하는 것이 깔끔하고 가독성이 좋아보입니다.

Comment on lines 35 to 44
<div style={{width: "30px"}}></div>
<div style={{
width: "30px",
textAlign: "center",
borderLeft: "3px solid #aaa",
lineHeight: "0.1em",
height: 500,
}}
>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

div 태그의 style속성에서 스타일을 지정하기 보다
class를 선언해 css 파일에 분리하는 것이 깔끔하고 가독성이 좋아보입니다.

Comment on lines 57 to 65
<div style={{width: "30px"}}></div>
<div style={{
width: "30px",
textAlign: "center",
borderLeft: "3px solid #aaa",
lineHeight: "0.1em",
height: 500,
}}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

div 태그의 style속성에서 스타일을 지정하기 보다
class를 선언해 css 파일에 분리하는 것이 깔끔하고 가독성이 좋아보입니다.

Comment on lines 60 to 68
<div style={{width: "30px"}}></div>
<div style={{
width: "30px",
textAlign: "center",
borderLeft: "3px solid #aaa",
lineHeight: "0.1em",
height: 500,
}}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

div 태그의 style속성에서 스타일을 지정하기 보다
class를 선언해 css 파일에 분리하는 것이 깔끔하고 가독성이 좋아보입니다.

Copy link
Owner

@whitecity01 whitecity01 left a comment

Choose a reason for hiding this comment

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

리액트를 많이 경험해보지 못한 상황임에도 불구하고 너도 훌륭하게 짠 코드인 것 같습니다!
지원님이 말하신 대로 더 나은 css 분리에 대해 고민해보시는 게 좋을 것 같고, 컴포넌트의 중복을 해결할 방법을 고심해 보시는 게 도움될 것 같습니다.
수고하셨습니다~

}}
>
</div>
<h1 className={`tab ${mode === 'stopwatch' ? 'active' : null}`}
Copy link
Owner

Choose a reason for hiding this comment

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

stopwatch일 경우에 active를 추가하면 되는 상황이기 때문에 ? 보다는 && 연산자를 사용하는 것이 가독성에 더 좋을 것 같습니다!

${mode === 'stopwatch' && 'active'}

Comment on lines 1 to 31
.container {
display: flex;
justify-content: space-between;
align-items: flex-start;
}

.content {
flex: 1;
text-align: center;
align-items: center;
flex-direction: column;
}
.time{
font-size: 40px;
}
.button {
display: flex;
justify-content: center;
gap: 5px;
margin-top: 10px;
}
.buttonImage {
width: 30px;
height: 30px;
object-fit: contain; /* 이미지가 버튼 크기에 맞게 조정되도록 */
}

.records {
flex: 1;
text-align: left;
} No newline at end of file
Copy link
Owner

Choose a reason for hiding this comment

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

프로젝트 규모가 커지면 어떤 컴포넌트의 어떤 기능의 className인지 파악하기가 힘들고 중복이 생겨 예기치 못한 상황이 발생할 수 있습니다. 따라서 컴포넌트별로 고유한 className을 부여하는 것이 좋고, scss의 중첩기능을 활용하는 것이 좋습니다.

</div>
</div>

<div style={{width: "30px"}}></div>
Copy link
Owner

Choose a reason for hiding this comment

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

이런 동일한 밑줄 코드가 자주 나오면 하나의 컴포넌트로 구현하여 사용하는 것도 좋은 방법입니다~

import './timer.css'

function Timer(){
const initialTime = 100000
Copy link
Collaborator

Choose a reason for hiding this comment

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

이건 예시를 전부 다르게 이해해서 그런거긴 하지만 입력받아서 처리하는 형태로 initial time을 usestate로 받아 처리해주면 더 좋을것 같습니다!!

</div>

<div className='records'>
<h2>기록</h2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

간단하게 h2 태그를 사용하는 것도 좋지만 css로 글자속성을 정의해줄 수 있다는 방법도 아시면 좋을것 같습니다.
왜냐면 h2 h1 태그 같은 경우엔 사용자가 마음대로 크기를 조정하는 것이 아니라 정해진 크기에 맞춰서 쓰는 것이기에 필요할 때가 있을 것 같네요.

@0andrich 0andrich changed the title week1 - 타이머/스톱워치 Feat : week2 - todolist Aug 7, 2024
@@ -0,0 +1,22 @@
import './ToDoListItem.css';
Copy link
Collaborator

Choose a reason for hiding this comment

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

app.js를 제외한 컴포넌트 파일들은 components라던지 다른 내부의 폴더로 관리해주면 더 좋을것 같습니다.!!

};

const remove = (id) => {
setTodos(todos.filter((todo) => todo.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.

filter함수 사용한게 너무 너무 기발한 아이디어인 것 같습니다!! 👍😁

import ReactDOM from 'react-dom/client';
import App from './App';

const root = ReactDOM.createRoot(document.getElementById('root'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

다음 과제인 라우터할땐 이파일도 만져야 하더라고요
이 정보가 도움이 되면 좋겠습니다.

Copy link
Owner

@whitecity01 whitecity01 left a comment

Choose a reason for hiding this comment

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

이전과제에 비해 많이 발전한 것 같습니다.
변수 네이밍과 css 구현 방식에 대해 조금 더 고민해보는 것이 도움될 것 같습니다!


return (
<div className="App">
<div className='AppBar'>일정관리</div>
Copy link
Owner

Choose a reason for hiding this comment

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

flutter의 냄새가 은은하게 퍼지는 좋은 클래스명 같습니다

<div className="App">
<div className='AppBar'>일정관리</div>
<div className='inputContainer'>
<input className="todoInput" type='text' placeholder='할 일을 입력하세요' value={input} onChange={(e)=>setInput(e.target.value)}></input>
Copy link
Owner

Choose a reason for hiding this comment

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

태그 사이에 들어갈 내용이 없다면 하나의 태그로 <input /> 작성하는 게 가독성 측면에서 좋습니다~

Comment on lines +6 to +20
return (
<div className="item">
<input className='checkbox'
type="checkbox"
checked={todo.checked}
onChange={() => checking(todo.id)}
/>

<span className='text'>{todo.text}</span>
<span className='button'>
<button onClick={() => remove(todo.id)}><HiTrash /></button>
</span>
</div>
);
}
Copy link
Owner

Choose a reason for hiding this comment

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

프로젝트 규모가 커질경우 곂칠 수 있는 className은 피하는 것이 좋습니다
todo-item이나 todo-list-item 과 같은 이름이 어떨까요?

Comment on lines +23 to +24
checking={checking}
remove={remove}
Copy link
Owner

Choose a reason for hiding this comment

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

이건 개인적인 의견인데, 어떤 것을 checking 하는지, 어떤 것을 remove 하는 지 각 함수의 기능을 짐작할 수 있는 네이밍을 하는 것이 좋을 것 같습니다!

};

const remove = (id) => {
setTodos(todos.filter((todo) => todo.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.

filter함수를 쓰니 훨씬 간단해지는군요 배워갑니다

@0andrich 0andrich changed the title Feat : week2 - todolist Feat : week3 - api Aug 13, 2024
Comment on lines +34 to +93
const plusTodo = async () => {
if(input.trim()){
const maxId = todos.length ? Math.max(...todos.map(todo => todo.id)) : 0;
const newTodo={
id:maxId+1, //maxId++이 안되는 이유는? maxId는 const로 바뀌면 안되기 때문
text:input,
done:false
};
try {
console.log(input);
const response = await axios.put(`${serverUrl}/todo/${newTodo.id}.json`, newTodo);
if(response.status === 200){
setTodos([...todos, newTodo]);
setInput('');
}
else {
console.log('Failed to put', response);
}
} catch (error) {
console.error('Error writing todo:', error);
}
}
}
const enterHandler = (e) => {
if (e.key === "Enter") {
plusTodo();
}
};

const todoChecking = async (id) => {
try {
const todoFind = todos.find(todo => todo.id === id);
if(todoFind){ //아무것도 없을 수 있기 때문
const doneTodo = { ...todoFind, done: !todoFind.done };
const response = await axios.patch(`${serverUrl}/todo/${id}.json`, doneTodo);
if (response.status === 200) {
setTodos(todos.map(todo => (todo.id === id ? doneTodo : todo)));
} else {
console.log('Failed to patch', response);
}
}

} catch (error) {
console.error('Error updating todo:', error);
}
};

const todoRemove = async (id) => {
try {

const response = await axios.delete(`${serverUrl}/todo/${id}.json`);
if (response.status === 200) {
setTodos(todos.filter(todo => todo.id !== id));
} else {
console.log('Failed to delete:', response);
}
} catch (error) {
console.error('Error deleting todo:', error);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

api를 호출하는 주 기능 함수들은 services디렉토리에 따로 분리한다면 좋을거같습니다~

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.

4 participants