-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FE] feat: 형광펜 메뉴 및 툴팁 구현 #874
Conversation
- 꺼짐 : 버튼 왼쪽 - 켜짐: 버튼 오른쪽
원인 :서버가 내려준 데이터 타입과 클라이언트 타입 불일치
API 오류 수정서버에서는 'ranges'로 주고 클라이언트에서는 'rangeList'로 다루는 것에 따른 타입 오류를 수정했어요. 🌟형광펜 정보에 따라 주관식 답변을 순차적으로 형광펜 적용 부분/미적용 부분으로 잘라야해서, 형광펜 rangeList는 endIndex를 기준으로 오름차순으로 정렬되어야합니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제보
모바일에서, *까지 포함해서 드래그한 뒤 형광펜을 치거나 삭제를 시도하면 해당 동작이 먹히지 않습니다. 노트북에서는 *를 포함해 드래그하면 형광펜 메뉴가 아예 뜨지 않고요. 지금 * 가 이미지로 들어가 있어서 어떻게 보면 당연한 현상이지만 사용성을 위해서는 *는 드래그할 수 없게 막아버리는 게 좋을 것 같습니다.
(모바일 영상)
https://github.com/user-attachments/assets/42448792-4bde-4725-9c4f-6299061f47cb
리뷰
PR 본문에 리팩토링의 가능성이 나와 있어서 전반적인 흐름...만 읽어봤습니다. (절대 핑계입니다.) RC는 위의 제보에 대해 얘기해보고 싶어서 남겼읍니다
저번 버전도 좋았는데, 드래그한 영역에 체크됐던 내역이 있다면 형광펜과 지우개를 둘 다 보여주는 개선된 UI가 여러 케이스를 커버할 수 있을 것 같아 더 편리해졌네요!
화면 크기별 툴팁 안내 문구가 다른 것도 좋고, 형광펜 on/off쪽 디자인도 펜 모양을 없애니까 더 깔끔해진 것 같아요.
건의
소소한 건의사항으로는, 모바일 환경에서 툴팁을 보면 좁은 width에 많은 글자가 뭉쳐 있다 보니 본문과 툴팁의 구분감이 약한 느낌이 들었어요. 그래서 그림자를 조금 더 진하게 주면 좋을 것 같아요!
또 모바일에서는 텍스트 왼쪽 정렬이 더 예쁠 것 같기도 합니당
@@ -3,16 +3,28 @@ import { useState } from 'react'; | |||
import { HIGHLIGHT_SPAN_CLASS_NAME, SYNTAX_BASIC_CLASS_NAME } from '@/constants'; | |||
import { SelectionInfo } from '@/utils'; | |||
|
|||
export type HighlightArea = 'full' | 'partial' | 'none'; | |||
|
|||
const useCheckHighlight = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 훅이 단순히 형광펜이 쳐진 영역을 리턴하는 게 아니라 형광펜이 쳐진 영역의 범위를 체크하는 훅이라, 이름에 부가적으로 Range 같은 정보가 들어가면 좋겠...지만 지금도 이해하는 데 어려움은 없습니다ㅎㅎ
$position={position} | ||
$width={HIGHLIGHT_BUTTON_SIZE.width.buttonWidthColor} | ||
> | ||
<S.Button onClick={addHighlightByDrag}> | ||
<span className={SR_ONLY}>하이라이트 추가 버튼</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뒷북이지만 😅 aria-label
등을 사용하는 대신 SR_ONLY
클래스를 별도로 설정한 이유가 궁금해용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당시에 접근성에 대한 합의가 이루어지지 않고 구조상 해당 버튼에 대한 설명이 안에 들어있는게 좋지 않나 생각했어서, 별로도 관리하는 방향으로 진행했어요.
지금은 aria-label만으로 충분하다고 생각해 이를 사용하는 것으로 변경되었어요.
frontend/src/types/highlight.ts
Outdated
@@ -17,12 +17,16 @@ export interface Highlight { | |||
// 서버에서는 ranges로 줌 | |||
rangeList: HighlightRange[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버와 속성명을 맞추지 못한 이유가 있었나욥?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프론트는 배열에 's'를 사용한 복수형을 쓰지 않기로 했지만, 백엔드쪽에서는 복수형을 사용중이라 타입명에서 차이가 나요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
부연설명을 하자면, 백엔드에서는 자료구조를 변수명에 담지 않습니다 ㅎㅎ.. str
, list
와 같은 것을 변수명에 적게 되면 나중에 변수 타입이 달라졌을 때 불일치할 수 있기 때문이예요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
설마 했는데 프리코스때 받았던 피드백과 같은 이유였군요ㅋㅋㅋㅋ
export const HIGHLIGHT_MENU_WIDTH: { [key in HighlightArea | 'longPress']: number } = (() => { | ||
return { | ||
partial: HIGHLIGHT_BUTTON_WIDTH * 2, | ||
none: HIGHLIGHT_BUTTON_WIDTH, | ||
full: HIGHLIGHT_BUTTON_WIDTH, | ||
longPress: HIGHLIGHT_BUTTON_WIDTH, | ||
}; | ||
})(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
왜 하이라이트된 영역별로 메뉴 너비를 따로 설정해주고 있지? 싶었는데 케이스에 따라 메뉴에서 보여줘야 하는 요소가 달라져서 그런 거군요ㅋㅋㅋㅋ
export const Message = styled.aside` | ||
position: absolute; | ||
/* HelperIcon과 말풍선 삼각형 높이 */ | ||
top: calc(1.6rem * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동적인 🔺이었군요
{isTouchDevice() ? ( | ||
<> | ||
<p>글자를 선택해 형광펜을 적용하거나 삭제할 수 있어요</p> | ||
<p>형광펜이 적용된 영역은 슬라이드 동작으로 삭제할 수 있어요</p> | ||
</> | ||
) : ( | ||
<> | ||
<p>드래그하여 형광펜을 적용하거나 삭제할 수 있어요</p> | ||
<p>형광펜이 적용된 영역은 길게 눌러 삭제할 수 있어요</p> | ||
</> | ||
)} | ||
</S.Message> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👏
onMouseMove={clearPressTimer} | ||
onTouchMove={handleLongPressLine} | ||
> | ||
<S.Marker src={DotIcon} alt="점" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 * 이미지에 대한 별도의 alt 설명이 필요할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
li의 마크와 답변 정렬이 안되는 오류가 있어서 임시방편으로 마크를 대신하는 이미지를 넣었고, 당시에는 답변 항목 구분자로 의미가 있다고 생각했어요.
현재는 마크와 답변 정렬 오류를 해결해, 이미지를 삭제했어요
- 글자 수에서 -1 안 한 부분 있었음
- 초기에 만든 block 변수명이 곳곳에 남아있어...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정사항 전부 확인했습니다~~ 툴팁 그림자는 이 정도만 해도 충분할 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
형광펜 메뉴에서 추가/삭제가 동시에 뜨니 확실히 사용하기도 편해졌고, 사용자 입장에서 여러 가지 케이스를 고민하지 않아도 돼서 좋은 것 같아요.
형광펜 오류.webm
로컬에서 실행해보다가 한 가지 버그를 발견해서 제보합니다! 덧칠할 때 순방향이면 영상과 같은 버그가 없지만 역방향일 때만 저러는 것 같아요.
버그는 알려야 할 것 같아서 RC 남깁니다!
import { HIGHLIGHT_BUTTON_SIZE } from '@/constants'; | ||
import { Position } from '@/types'; | ||
|
||
export const Menu = styled.div<{ $position: Position; $width: number }>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링 시 style props를 interface로 정의하면 될 것 같아요
<S.TooltipButton onMouseOver={() => setIsOpenMessage(true)} onMouseOut={() => setIsOpenMessage(false)}> | ||
<S.HelperIcon src={HelperIcon} alt="도움말 아이콘" /> | ||
{isOpenMessage && ( | ||
<S.Message role="tooltip"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
role 속성도 함께 챙겨주었네요👍
`; | ||
export const AnswerText = styled.div` | ||
margin-left: 8px; | ||
margin-left: 0.8rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지난 커밋에서 margin-left 값을 0.8rem에서 2rem으로 수정했었는데, 이 브랜치에서 확인해 보니 0.8rem일 때도 너무 달라붙어 있다는 느낌이 안 드네요...!
이 부분이 충돌될 것 같아서 코멘트 남깁니다! 0.8rem도 괜찮으면 이대로 가도 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 답변과 마크가 일렬로 되어있어서 0.8rem도 괜찮은 것 같아서 0.8rem으로 갈게요
&::marker { | ||
margin: 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list style 말고도 ::marker
속성을 사용할 수 있군요 :0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코멘트 확인 부탁해요 :)
…eview-me into fe/feat/872/highlight-area-tooltip
오류2.webm |
- 잘못된 변수로 계산함
찐막 형광펜 버튼 위치 오류 수정
_2024_10_21_23_12_31_720.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
탈도 많고 기능도 많은 형광펜 메뉴 구현이 드디어 끝났네요! 오류 없는 것 확인했습니다. 고생했어요
* refactor: line 관련 변수명 변경 (blockElement -> lineElement) * feat: HighlightMenu 컴포넌트 훅 생성 및 기존 관련 코드 변경 * fix: 하이라이트 적용과 미적용 같이 있을 때, isForwardDrag 값 오류 수정 * refactor: 하이라이트 메뉴 사이즈 관련 변수 수정 * design: 하이라이트 메뉴 버튼 스타일 변경 (사이즈 조정 및 hover시 배경색 변경) * fix: 모아보기 페이지- 주관식 답변 개행에도 li marker 표시되는 오류 수정 * refactor: 하이라이트 메뉴 위치 초기화하는 함수명 변경 * fix: 길게 눌렀을 때 삭제되는 버튼 안 뜨는 오류 수정 및 상태명 변경 * feat: 형광펜 기능 알려주는 툴팁 구현 * chore: 불필요한 콘솔 삭제 * fix: useMutateHighlight 테스트 오류 수정 * design: 형광펜 스위치 버튼 디자인 수정 - 꺼짐 : 버튼 왼쪽 - 켜짐: 버튼 오른쪽 * fix: 리뷰 모아보기 데이터 타입 차이로 인한, 형광펜 적용 안되는 오류 수정 원인 :서버가 내려준 데이터 타입과 클라이언트 타입 불일치 * design: EditorLineBlock 스타일 컴포넌트 삭제 * chore: 리뷰 모아보기 목 데이터 변경(형광펜 적용 추가) * chore: 불필요한 코드 삭제 * feat: 리뷰 모아보기- 주관식 답변 Dot 선택되지 않게 처리 및 UndraggableWrapper에 min-width 적용 * feat: 형광펜 메뉴 위치 - 에디터 상단 넘는지 확인하는 기능 추가 * design: 툴팁 그림자, 문자 졍렬 변경 * chore: 주석처리 제거 * chore: sr-only 삭제 * chore: 답변 항목 구문자 alt 빈문자열로 변경 * design: 그림자 폭 수정 * fix:span 자동 줄넘김 오류 수정 * fix : list 마크와 답변 정렬 안되는 오류 수정 및 마크 이미지 삭제 * chore: 불필요한 콘솔 삭제 * refactor: block -> line으로 변경 * fix: 형광펜 더하기 시, endIndex 오류 수정 - 글자 수에서 -1 안 한 부분 있었음 * docs: 리뷰 모아보기 페이지 하이라이트 목 데이터 변경 * style: Syntax에 prettier 적용 * fix: 글자의 마지막에만 형광펜 칠해지지 않는 오류 수정 * fix: 형광펜 있는 영역을 포함한 여러 답변에서 형광펜 더할 때 offset 오류 수정 * refactor: block -> line으로 변경 - 초기에 만든 block 변수명이 곳곳에 남아있어... * chore: 하이라이트 목 핸들러 필요 없는 response 삭제 * fix: 하이라이트 API오류 시, fallback 실행으로 isEditable 세션스토리지 값 삭제되는 오류 수정 * chore: 불필요한 타입(Highlight 삭제) * fix: 같은 답변 다른 줄, 드래그 방향 오류 수정 * fix: 같은 답변 다른 줄 드래그 방향 계산 오류 수정 - 잘못된 변수로 계산함
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
Screen_Recording_20241018_164946_Samsung.Internet.mp4
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말