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

Feature/menu text editor #2

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

Conversation

shon5544
Copy link
Member

@shon5544 shon5544 commented Sep 1, 2022

Menu 화면과 텍스트 편집기

요약

만들어져있는 것은 Menu 컴포넌트와 텍스트 편집기입니다.
일단 텍스트 편집기의 기능은 완성했지만 제가 디자인을 잘못보고 만들어서 디자인은 다시 수정해서 push해야겠네요.

계획

앞으로 필요하다면 (거의 99% 필요할 것 같습니다만) Menu 컴포넌트의 부분(프로필 부분, 검색창 부분, 등등..)을 컴포넌트 화 시킬 생각입니다.

현재 MainScreen.tsx에서 text editor역할을 하는 컴포넌트는 CustomToolbar와 ReactQuill입니다. 이 둘을 묶어 새로운 컴포넌트를 만들어 재사용 가능하게 할 예정입니다.

리뷰 받을 점

  • 디렉토리 관리

현재 디렉토리 구성을 src 폴더에 화면의 부분만 차지하는 컴포넌트들은 Components 폴더로, 한 화면 전체를 구성하는 큰 컴포넌트는 Screen 폴더에 저장하고 관리하고 있습니다. 그 속에서 또 용도에 따라 또 폴더를 만들어 사용하는데, 이런 디렉토리 관리 법이 괜찮을지 걱정이 되는데 괜찮을까요? 더 좋은 방법이 있을까요?

  • 타입

우선 현재 MainScreen.tsx에서 만든 변수들 ( modules, formats )은 TypeScript가 직접 타입을 유추할 수 있도록 자세한 type은 만들지 않았습니다. 간단한 변수라도 무조건 타입을 만들어주는게 좋을까요??

그 밖에 영 아니다 싶은거 있으면 말씀해주세요! 바로 반영하겠습니다

<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

README라서 아직은 크게 상관없지만, 컨플릭트 해결한 뒤에는 이런 표시 지워야 할 것 같아요!

@@ -0,0 +1,43 @@
<!DOCTYPE html>
<html lang="en">
Copy link
Member

Choose a reason for hiding this comment

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

대부분 별 일 없긴 한데, 언어 설정 ko로 하는 게 더 좋아 보여요!

/* text-align: center; */
width: 100%;
height: 100%;
/* height: ; */
Copy link
Member

Choose a reason for hiding this comment

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

나중에 필요한 주석이 아니라면 지우는 게 덜 헷갈릴 듯해요!


@font-face {
font-family: 'GothicA1-Light';
src: url('https://cdn.jsdelivr.net/gh/projectnoonnu/[email protected]/GothicA1-Light.woff2') format('woff2');
Copy link
Member

Choose a reason for hiding this comment

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

jsdelivr를 쓰면 편하긴 한데 좀 느릴 거에요 아마! 파일을 직접 이용하거나 google font처럼 최적화된 방법을 쓰는 CDN을 쓰는 방법을 고려해 봐야 할 것 같아요

#Menu-container{
width: 211px;
height: 100%;
background-color: #F8F8F8;
Copy link
Member

Choose a reason for hiding this comment

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

Figma에 정의되어 있는 디자인 토큰을 CSS 변수로 선언하고 사용해 보는 건 어떨까요?

html {
  --background-color: #F8F8F8;
}

#Menu-container {
  background-color: var(--background-color);
}

@@ -0,0 +1,171 @@
#Menu-container{
Copy link
Member

Choose a reason for hiding this comment

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

CSS 네이밍 규칙을 정확히 세우는 게 좋을 것 같아요! 보통은 kebab-case로 많이 쓰는데, CSS Module의 경우에는 또 camelCase를 권장하는 등의 차이가 있어요


const Menu = () => {
return(
<div id='Menu-container'>
Copy link
Member

Choose a reason for hiding this comment

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

CSS Module이나 다른 스타일링 관련 솔루션(styled-components, emotion, ...)을 써보면 어떨까요? 이렇게 해도 문제 없는데, class나 id가 다른 컴포넌트와 중복될 가능성을 자꾸 고려해야 하니 실수가 나기 쉬워서, 편의상 자동으로 안 겹치는 이름을 생성해 주는 도구를 쓰면 어떨까 제안해 봅니다!

Comment on lines +25 to +38
const modules = {
toolbar: {
container: "#toolbar"
}
}

const formats = [
//'font',
'header',
'bold', 'italic', 'underline', 'strike', 'blockquote',
'list', 'bullet', 'indent',
'link', 'image',
'align', 'color', 'background',
];
Copy link
Member

Choose a reason for hiding this comment

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

컴포넌트 내부에서 변할 일 없는 이런 상수도 컴포넌트 내부에 선언하면 매 리렌더링마다 참조값이 바뀌어서 불필요한 리렌더링을 발생시킬 수 있어요! 컴포넌트 바깥에서 선언하거나 useMemo를 이용하면 더 좋을 것 같아요!

const modules = {
  toolbar: {
    container: '#toolbar',
  }
};

const formats = [
  'header',
  'bold', 'italic', 'underline', 'strike', 'blockquote',
  'list', 'bullet', 'indent',
  'link', 'image',
  'align', 'color', 'background',
];

const MainScreen = () => (
  // ...

@aube-dev aube-dev mentioned this pull request Sep 9, 2022
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.

2 participants