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

#773 채팅 140자 넘으면 입력 막기 & 글자 수 circular progress bar로 표시 #777

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

jinhyeonkwon
Copy link
Contributor

@jinhyeonkwon jinhyeonkwon commented Mar 26, 2024

Summary

It closes #773

140자 길이 제한을 구현하고, 향후 길이 제한을 바꿀 수 있도록 하였으며, react-circular-progressbar를 사용하여 현재 글자 수를 보기 쉽게 나타냅니다.

progressbar의 위치, 크기, margin 등이 과연 적절한가, 전체 리디자인 예정이기는 해도 style 변경 사항이 디자인 시스템의 체계를 너무 갖추지 않은 건 아닌가, state 사용이나 기타 등등 코드의 전체적인 구조와 맞지 않는 점은 없는가, 등을 중점적으로 리뷰해주시면 감사하겠습니다!

앞으로 최대 글자 수를 수정하려면?

  1. packages/web/src/tools/regExpTest.js 의 chatMsgLength 정규 표현식을 수정합니다.
  2. 이에 더해, packages/web/src/components/Chat/MessageForm/InputText/BodyText.tsx 의 maxChatMsgLength 변수 값도 함께 수정해 줍니다.

코드 수정 사항 설명

최신 버전 (2024.07.07.)

props에 대한 변경 사항을 모두 없애고, BodyText.tsx에서만 해결할 수 있도록 수정하였습니다.
리뷰 내용을 적용하면서, Progress Bar가 아랫쪽에 정렬되게 할 방법을 고민한 결과. BodyText.tsx와 index.tsx의 수정사항을 모두 없앤 후에, (정규 표현식 등의 변경 사항은 그대로입니다) 아래의 사항을 적용하였습니다.

  1. chatMsgLength라는 state와 maxChatMsgLength라는 변수를 BodyText 안에 선언합니다.
  2. CircularProgressBar를 BodyText에서 사용합니다. 즉 이제 BodyText 안에 CircularProgressBar, 입력 칸, ButtonSend가 함께 들어있게 된 것입니다. CircularProgressBar를 여기 넣게 되면서, Style 수정이 일부 들어갔습니다. 새롭게 디자인한 style들은 모두 css-in-js를 적용하도록 맞추었습니다.
  3. onChange에서 현재 글자 수와 maxChatMsgLength를 비교하여, 넘어가면 최대 글자수에 맞게 자릅니다. (연속 입력이나 긴 텍스트 붙여넣기의 경우에도 최대 글자수(현재 140)에 맞게 조정됩니다)
  4. CircularProgressBar는 chatMsgLength state 값에 따라 그림을 그립니다.

(이전 버전 (~봄학기))

packages/web/src/components/Chat/MessageForm/InputText/index.tsx
의 InputText 안에 정의된 maxChatMsgLength 길이를 넘지 못하도록 합니다.
이를 구현하기 위해, BodyText의 BodyTextProps를 변경하여, 부모가 자식인 BodyText에게 본인의 함수를 넘겨줌으로써 textarea 안의 내용이 변경될 때마다 부모 안에 정의된 progress bar가 바뀌도록 하였습니다. 또한 BodyText에서 props로 max 길이를 전달 받아, index.tsx에서의 maxChatMsgLength 변경만으로 모든 로직에 max 길이의 변경 사항이 반영되도록 하였습니다.

index.tsx의 style 구조를 조금 많이 바꿨습니다. flexbox를 사용하여 progress bar와 입력 칸이 가로로 배치되도록 하는 과정에서 변화가 있었습니다

Images or Screenshots

최신 디자인

(네이비색 점은 제 크롬 확장 프로그램 때문이라 무시하셔도 됩니다)
학기 중 회의 결과 (bar 두께 줄이기, 아래로 정렬, 글자 수 표시하지 않기) 반영
image
image

(이전 디자인 기준)

아래와 같이 react-circular-progressbar를 사용하여 140자 제한 중 얼마나 사용했는지 보여줍니다.
image

그리고 140자가 넘으면 이렇게 되고 입력이 더이상 되지 않습니다. 140이라는 숫자는 위에 서술한 것과 같이, 바꿀 수 있습니다. 글자수 정책 변경 시, 앞으로는 정규식 외에도 이 부분을 함께 수정해주어야 합니다.
image

Further Work

  • Do something...

Copy link

netlify bot commented Mar 26, 2024

Deploy Preview for taxi-dev-preview ready!

Name Link
🔨 Latest commit 20526aa
🔍 Latest deploy log https://app.netlify.com/sites/taxi-dev-preview/deploys/668a2b4d568d0700081a4a2a
😎 Deploy Preview https://deploy-preview-777--taxi-dev-preview.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.

@jinhyeonkwon jinhyeonkwon changed the title #773 채팅 140자 넘으면 입력 막기 글자 수 circular progress bar로 표시 #773 채팅 140자 넘으면 입력 막기 & 글자 수 circular progress bar로 표시 Mar 26, 2024
Comment on lines +4 to +5
const chatMsg = (x) => RegExp("^\\s{0,}\\S{1}[\\s\\S]{0,}$").test(x);
const chatMsgLength = (x) => RegExp("^[\\s\\S]{1,140}$").test(x);
Copy link

Choose a reason for hiding this comment

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

@jinhyeonkwon 흠... ^\\S{1}[\\s\\S]{0,139}$
const chatMsg = (x) => RegExp("^\\s{0,}\\S{1}[\\s\\S]{0,}$").test(x) && RegExp("^[\\s\\S]{1,140}$").test(x);

로 통합할 수 있지 않을까요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 @xMHW 님의 확인 부탁드리겠습니다..!

Copy link

Choose a reason for hiding this comment

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

통합해도 상관없습니다~ 그냥 명시적으로 역할이 달라서 구분해놓았습니다. 위에 regex는 빈칸만 있는 것 제외하는 용도이고, 아래는 글자수제한입니다.

Comment on lines 30 to 35
style={{
display: "flex",
flex: 1,
flexDirection: "row",
alignItems: "center",
}}
Copy link

Choose a reason for hiding this comment

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

저희는 emotion을 사용합니다 !!
css Prop을 사용해주세요!!

style prop에 object로 스타일을 넘기면 react에서 다른 prop 이 정의된 걸로 이해해서
매번 리렌더링 됩니다.. 성능 이슈!!

emotion을 사용하면 css-in-js 여서 오브젝트로 넘겨도 해싱된 className으로 변환해 주어서 괜찮습니다 !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 그렇군요.. 관련하여 css 부분 수정하도록 하겠습니다..!

Comment on lines 38 to 43
style={{
width: "28px",
height: "28px",
marginLeft: "0px",
position: "relative",
}}
Copy link

Choose a reason for hiding this comment

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

마찬가지!

import { useState } from "react";
import { CircularProgressbar, buildStyles } from "react-circular-progressbar";
import "react-circular-progressbar/dist/styles.css";
import { Direction } from "react-toastify/dist/utils";
Copy link

Choose a reason for hiding this comment

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

허걱 얜 뭐죠???

})}
/>
</div>
<div style={{ flex: 1 }}>
Copy link

Choose a reason for hiding this comment

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

인라인 스타일 문제점 동일 !

@@ -1,3 +1,8 @@
import { useState } from "react";
import { CircularProgressbar, buildStyles } from "react-circular-progressbar";
import "react-circular-progressbar/dist/styles.css";
Copy link

Choose a reason for hiding this comment

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

흠... 저의 개인적인 생각인데.. 원형 바 만드는데 라이브러리까지 쓸 필요가 있나 싶네요..ㅋㅋㅋ 하하

Copy link
Contributor Author

Choose a reason for hiding this comment

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

가장 쉬운 방법을 사용하긴 했는데, 다른 선택지도 있으면 환영합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

기존에 있는 라이브러리 쓰는 것도 방법일 것 같아요ㅎㅎ

@GKim67
Copy link

GKim67 commented Mar 27, 2024

@jinhyeonkwon @predict-woo 허걱 넷틀리파이 빌드 에러 뜨네요 !!
이거 해결해야 할듯?? 아마 별 문제는 아니고 선언했는데 안쓰는 변수 있거나 type 에러거나 사소한 것 아닐까싶네요..??

@kmc7468
Copy link
Member

kmc7468 commented Mar 27, 2024

왼쪽이랑 오른쪽 둘 다 빈 공간이 생기는데.. 이걸 해결할 괜찮은 방법이 없을까요?

Progress Bar는 세로축 기준으로 가운데 정렬인데, 전송 버튼은 아래에 고정되어 있는 것도 조금 부자연스럽게 느껴지는 것 같습니다.

인태님께 디자인 관련 피드백 요청드려도 괜찮을 것 같습니다! 작업 수고하셨습니당

@jinhyeonkwon
Copy link
Contributor Author

빌드 에러가 아마 Direction을 안 쓰는 것 때문인 것 같은데, 다음 수정 사항에 저 import를 아예 지워보겠습니다!

@@ -82,6 +93,15 @@ const BodyText = ({ sendMessage }: BodyTextProps) => {
if (isSendingMessage) refreshTextArea();
setIsMessageValidState(getIsMessageValid(textareaRef.current.value));

if (textareaRef.current.value.length > maxChatMsgLength) {
Copy link
Contributor

Choose a reason for hiding this comment

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

regExpTest.chatMsgLength와 같은 역할 아닌가용? 중복된 로직같아서 여쭤봅니다ㅎㅎ

Choose a reason for hiding this comment

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

@GrasshopperBears 헉 오랜만에 리뷰하신거 보니 반갑구만요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

너무 늦게 확인했네요ㅠㅠ 리뷰 감사합니다!
정규식을 다시 활용하면 BodyText에서 max 길이를 props로 받아올 필요가 없겠네요! 반영하여 수정하겠습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

검사 로직은 기존의 것을 사용하는 것으로 수정하였고, substring 부분의 경우 regExpTest.js에 새로운 함수를 도입하여 match 를 사용하는 것을 시도하였으나 버그가 있어서 우선 검사 로직만 수정한 커밋 올렸습니다
BodyText의 props에서 maxChatLength를 아예 제외한다고 해도, CircularProgressBar때문에 InputText의 index.tsx에 max 길이 변수가 존재해야 하기 때문에, 전달되는 props가 하나 줄어드는 차이 뿐임을 생각하면, 이 정도로 두어도 괜찮을 것 같기도 한데 다들 어떻게 생각하시는지 궁금합니다!

Copy link
Contributor

@GrasshopperBears GrasshopperBears Apr 24, 2024

Choose a reason for hiding this comment

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

제가 본 부분에 한해서 동작상에 문제는 없을 것 같아 일정이 있다면 이하 커멘트는 무시하셔도 괜찮습니다ㅎㅎ

추가적인 커멘트로는

  • (원래 커멘트의 의도) if절 앞 getIsMessageValid에서도 길이 검사를 하는데 여기서 또 한다. 어떻게 메서드를 분리하거나 하나를 생략할 수는 없을지?
  • @jinhyeonkwon 님의 커멘트에서 (개인적으로는) props 전달은 최소화되면 좋다고 생각합니다. 하나 정도는 괜찮지가 쌓여서 props 20개씩 될 수도 있는 거고 그러면 관리가 복잡해지는... 또 역할이라는 측면에서도 생각해볼 수 있을 것 같아요. BodyText의 역할은 무엇인지? 만약 BodyText의 역할이 순수한 텍스트의 표시라면 maxChatLength를 전달해 최대 길이 제한을 여기서 하는 게 맞을까? 길이 제한은 래핑하는 컴포넌트로 할 수도 있지 않을까? (이렇게 한다면 depth가 깊어지는 문제가 생기겠군요; 생각해보니 별로인 것 같은 아이디어인데 암튼) 답은 없을테니 팀원들이랑 잘 고민해보세요ㅎㅎ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

채팅 140자 넘으면 입력 막기, 글자 수 circular progress bar로 표시
6 participants