feat: `Panel` 컴포넌트 구현 (#36) #56

Puterism · 2021-07-09T12:06:37Z

기능 목록

Panel 컴포넌트 구현

기타

utils/color.ts에 배경색에 따른 텍스트 색상을 반환하는 로직을 작성함
- 배경색이 어두운 계열일 경우 텍스트 색상을 white, 밝은 계열일 경우 black을 반환함

API

`Panel`

Property	Description	Type	Default
`expandable`	해당 속성이 `true`일 경우 `Panel`을 펼쳤다가 닫을 수 있는 UI로 변경된다.	`boolean?`	`false`
`initialExpanded`	`Panel`이 초기에 펼쳐져 있는 상태인지 정의한다. 해당 속성이 `true`일 경우 `Panel` 컴포넌트가 처음 렌더될 때부터 펼쳐져 있는 형태로 보여준다. `expandable`이 `true`일 때만 동작한다.	`boolean?`	`false`
`children`	`Panel.Header`로 `Panel`의 헤더 영역, `Panel.Content`로 `Panel`의 내용 영역을 정의할 수 있다.	`ReactNode`

`Panel.Header`

Property	Description	Type	Default
`bgColor`	배경 색상을 지정한다. 배경 색에 따라 텍스트 색상이 검은색 혹은 흰색으로 자동으로 변경된다.	`string?`
`children`		`ReactNode`

`Panel.Content`

Property	Description	Type	Default
`children`		`ReactNode`

close #36

yujo11

코드 잘 봤습니다 쳎!

storybook으로도 확인해봤는데 엄청 깔끔하네요!

코멘트 남긴거 한번만 확인해주심 감사하겠습니다! 고생하셨습니다!! 👍👍👍

yujo11 · 2021-07-09T12:31:45Z

frontend/src/utils/color.ts

+// ITU-R BT.709 formula
+const getLuma = ([red, green, blue]: number[]) => 0.2126 * red + 0.7152 * green + 0.0722 * blue;
+
+export const getTextColor = (backgroundColor: string) => {


오 이 유틸 함수 덕분에 앞으로 Contrast 맞추기가 쉬워지겠네요!! 👍👍👍

yujo11 · 2021-07-10T12:42:38Z

frontend/src/components/Panel/PanelContext.ts

+const PanelContext = createContext<PanelProps>({
+  expandable: true,
+  expanded: false,
+});


오 Context를 활용한게 정말 멋지네요! 👍👍👍

yujo11 · 2021-07-10T12:42:50Z

frontend/src/components/Panel/PanelContext.ts

+if (process.env.NODE_ENV !== 'production') {
+  PanelContext.displayName = 'PanelContext';
+}


이 부분은 혹시 어떤 역할을 하고 있는지 알 수 있을까요?

React Devtools로 Context 관련 내용을 확인하려고 하면 모두 Context.Provider로만 보여지기 때문에 어떤 Context인지 네이밍을 확실히 해주기 위해 적용한 코드입니다.

여기에서 참고했습니다.

SunYoungKwon

재사용성을 많이 고려한 컴포넌트라 너무 좋습니다ㅎㅎ 고민을 많이 한게 느껴져요!

패널 열고 닫는 이벤트를 컴포넌트 사용자 측에서 주입하는 것이 아닐테니, 나중에 따로 issue를 만들기보다 이번 PR에 같이 포함하는게 좋을 것 같은데 어떠신가요??

SunYoungKwon · 2021-07-10T12:56:03Z

frontend/src/components/Panel/PanelContent.tsx

+  const { expandable = false, expanded = false } = useContext(PanelContext);
+
+  return (
+    <>{(!expandable || (expandable && expanded)) && <Styled.Content>{children}</Styled.Content>}</>


조건이 좀 길어서 변수로 분리하는 것을 고려해봐도 좋을 것 같아요

expandable하지 않은 Panel이면 항상 내용을 보여주고, expandable한 패널이면서 펼쳐져 있는 상태면 보여준다는 조건문인데 어떤 네이밍으로 분리해야할 지 고민이 되네요...
차라리 조건문을 나눠서 if문으로 early return을 하는 게 더 나을 수 있겠네요.

피드백 감사합니다! 수정사항 반영하고 다시 리뷰 요청 드릴게요~

isShowContent나 isDisplayContent 같은건 어떨까요...?

그러고보니 저는 boolean타입의 변수에 is나 has를 붙여주는 편이에요.
disabled 어트리뷰트나 이 프로젝트가 typescript를 사용중인걸 고려하면 상관없을 것 같기도 하고, 컨벤션으로 맞추는게 좋을 것 같기도 하고 애매하네요ㅠㅠ

@yujo11 생각은 어때요??

boolean의 경우에 무조건 is나 has를 붙여주는 건 헝가리안 표기법이 아닐까 하는 생각이 들었어요. 그래서 저는 안 붙이는 것도 좋아보여요
이와 별개로 해당 부분은 컨벤션이 없어서 혼란이 있을 것 같은데, 이것도 정하면 좋을 것 같습니다 :)

개인적으로 이런 방식으로 작성된 코드를 예시코드로 본 적이 있어서 그렇게 위화감이 크진 않아요. 하지만 이번 경우엔 전의 코드가 더 나은 것 같아요ㅋㅋㅋ😂 번거롭게 해서 죄송합니당ㅠ

부분적으로 loading이나 error 상태를 표시하거나, '검색 결과가 존재하지 않습니다'같은 메세지를 띄우는 상황 등에서 요런 방식을 사용하면 좋을 것 같네요!

isShowContent나 isDisplayContent 같은건 어떨까요...?

그러고보니 저는 boolean타입의 변수에 is나 has를 붙여주는 편이에요.
disabled 어트리뷰트나 이 프로젝트가 typescript를 사용중인걸 고려하면 상관없을 것 같기도 하고, 컨벤션으로 맞추는게 좋을 것 같기도 하고 애매하네요ㅠㅠ

@yujo11 생각은 어때요??

저 역시 헝가리안 표기법을 선호하지 않습니다! is, has 등의 prefix 없이도 구문에 어색함이 없다면 붙이지 않는 것도 좋을거 같아요!

SunYoungKwon · 2021-07-10T13:03:03Z

frontend/src/components/Panel/PanelHeader.styles.ts

+  color: ${({ theme, bgColor = 'transparent' }) =>
+    getTextColor(bgColor) === 'black' ? theme.black[500] : theme.white};


앞으로 여기저기서 많이 쓰일 것 같은데 한 번 더 추상화하는건 어떨까요??

좋은 아이디어네요!
일단 공통적으로 사용할 수 있을만한 스타일 정의를 어디에 두면 좋을지 추후 논의가 필요해보입니다.
우선은 같은 파일에 responsiveTextColor 라는 이름으로 스타일 정의만 따로 분리해둘게요.

다음 데일리에서 같이 논의해보시다!

좋아요~~

SunYoungKwon · 2021-07-10T13:05:11Z

frontend/src/components/Panel/PanelContext.ts

+const PanelContext = createContext<PanelProps>({
+  expandable: true,
+  expanded: false,
+});


context를 전역으로만 사용하지 않는다는 것은 알았지만, 실제 사용된 예시는 처음봐요! 배워갑니다~~👍👍👍

Material UI의 코드를 참고했습니다 :)

SunYoungKwon · 2021-07-10T13:06:01Z

frontend/src/utils/color.ts

+// ITU-R BT.709 formula
+const getLuma = ([red, green, blue]: number[]) => 0.2126 * red + 0.7152 * green + 0.0722 * blue;
+
+export const getTextColor = (backgroundColor: string) => {


크~~~👍👍👍👍👍

SunYoungKwon · 2021-07-10T13:14:58Z

PR에 사용법 문서화까지 완-벽-

Puterism · 2021-07-10T14:33:42Z

패널 열고 닫는 이벤트를 컴포넌트 사용자 측에서 주입하는 것이 아닐테니, 나중에 따로 issue를 만들기보다 이번 PR에 같이 포함하는게 좋을 것 같은데 어떠신가요??

좋은 생각입니다 :D
반영하고 다시 리뷰 요청 드릴게요

- 텍스트 색에 적용되는 `responsiveTextColor` 정의 - SVG fill 속성에 적용되는 `responsiveFill` 정의 - 공통적으로 적용할 수 있는 `ResponsiveColorProps` interface 정의

Puterism · 2021-07-10T15:37:11Z

@SunYoungKwon @yujo11
API 문서 수정하고 피드백 반영했습니다! 확인 부탁드려요 :D

yujo11

확인 했습니다! 완전 빠르네요 쳎!! 🚀🚀🚀

고생하셨습니다!! 👍👍👍

* chore: `prettier` 관련 ESLint plugin 및 config 설치 및 적용 * chore: `.gitignore`에 `.eslintcache` 추가 * fix: `ESLint` warning 및 error이 발생하는 코드 수정

- `expanded` prop을 `initialExpanded`로 이름 변경

Puterism · 2021-07-12T07:27:37Z

@SunYoungKwon 의 제안으로 컴포넌트 자체에서 상태를 가지도록 로직을 수정했습니다.
338a54c 에서 반영하였으니 확인 부탁드려요~

expanded -> initialExpanded 로 변경
onToggle prop 삭제

@SunYoungKwon @yujo11

SunYoungKwon · 2021-07-12T07:28:27Z

frontend/src/components/Panel/Panel.tsx

-    if (expandable && onToggle) {
-      onToggle();
+    if (expandable) {
+      setExpanded((prev) => !prev);


오~ 깔끔!!

깔끔22 👍👍

yujo11

고생하셨습니다 쳎!! 👍👍👍

yujo11 · 2021-07-12T07:33:46Z

frontend/src/components/Panel/Panel.tsx

-    if (expandable && onToggle) {
-      onToggle();
+    if (expandable) {
+      setExpanded((prev) => !prev);


깔끔22 👍👍

Puterism and others added 2 commits July 8, 2021 19:18

chore: cross-env 패키지 추가 및 적용 (#40)

b944b24

feat: Panel 컴포넌트 구현

e17986c

Puterism added 🌟 feature New feature or request 📚 frontend labels Jul 9, 2021

Puterism requested review from SunYoungKwon and yujo11 July 9, 2021 12:06

Puterism self-assigned this Jul 9, 2021

Puterism changed the title ~~feat: Panel 컴포넌트 구현 (#36)~~ feat: Panel 컴포넌트 구현 (#36) Jul 9, 2021

Puterism mentioned this pull request Jul 9, 2021

feat: ReservationListItem 컴포넌트 구현 (#35) #58

Merged

1 task

yujo11 approved these changes Jul 10, 2021

View reviewed changes

SunYoungKwon reviewed Jul 10, 2021

View reviewed changes

Puterism added 2 commits July 10, 2021 23:49

refactor: 배경색에 따라 색이 정해지는 스타일 정의를 분리

9ad8057

- 텍스트 색에 적용되는 `responsiveTextColor` 정의 - SVG fill 속성에 적용되는 `responsiveFill` 정의 - 공통적으로 적용할 수 있는 `ResponsiveColorProps` interface 정의

refactor: Panel 컴포넌트에 onToggle prop 추가

c1b8af4

yujo11 approved these changes Jul 11, 2021

View reviewed changes

Puterism and others added 2 commits July 12, 2021 16:07

fix: ESLint가 올바르게 동작하지 않는 문제 수정 (#70) (#72)

623ba6f

* chore: `prettier` 관련 ESLint plugin 및 config 설치 및 적용 * chore: `.gitignore`에 `.eslintcache` 추가 * fix: `ESLint` warning 및 error이 발생하는 코드 수정

refactor: Panel 컴포넌트가 expanded 상태를 가지도록 수정

338a54c

- `expanded` prop을 `initialExpanded`로 이름 변경

SunYoungKwon approved these changes Jul 12, 2021

View reviewed changes

yujo11 approved these changes Jul 12, 2021

View reviewed changes

Puterism merged commit 87ea0b0 into dev Jul 12, 2021

Puterism deleted the 36-panel branch July 12, 2021 07:58

Puterism mentioned this pull request Jul 12, 2021

fix: PanelContext의 타입이 맞지 않는 문제 수정 (#76) #77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `Panel` 컴포넌트 구현 (#36) #56

feat: `Panel` 컴포넌트 구현 (#36) #56

Puterism commented Jul 9, 2021 •

edited

Loading

yujo11 left a comment

yujo11 Jul 9, 2021

yujo11 Jul 10, 2021

yujo11 Jul 10, 2021

Puterism Jul 10, 2021

SunYoungKwon left a comment

SunYoungKwon Jul 10, 2021

Puterism Jul 10, 2021

SunYoungKwon Jul 10, 2021 •

edited

Loading

Puterism Jul 10, 2021

Puterism Jul 10, 2021

SunYoungKwon Jul 10, 2021

yujo11 Jul 11, 2021

SunYoungKwon Jul 10, 2021

Puterism Jul 10, 2021

SunYoungKwon Jul 10, 2021

yujo11 Jul 11, 2021

SunYoungKwon Jul 10, 2021

Puterism Jul 10, 2021

SunYoungKwon Jul 10, 2021

SunYoungKwon commented Jul 10, 2021

Puterism commented Jul 10, 2021

Puterism commented Jul 10, 2021

yujo11 left a comment

Puterism commented Jul 12, 2021 •

edited

Loading

SunYoungKwon Jul 12, 2021

yujo11 Jul 12, 2021

yujo11 left a comment

yujo11 Jul 12, 2021

		color: ${({ theme, bgColor = 'transparent' }) =>
		getTextColor(bgColor) === 'black' ? theme.black[500] : theme.white};

feat: Panel 컴포넌트 구현 (#36) #56

feat: Panel 컴포넌트 구현 (#36) #56

Conversation

Puterism commented Jul 9, 2021 • edited Loading

기능 목록

기타

API

Panel

Panel.Header

Panel.Content

yujo11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunYoungKwon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunYoungKwon Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunYoungKwon commented Jul 10, 2021

Puterism commented Jul 10, 2021

Puterism commented Jul 10, 2021

yujo11 left a comment

Choose a reason for hiding this comment

Puterism commented Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujo11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: `Panel` 컴포넌트 구현 (#36) #56

feat: `Panel` 컴포넌트 구현 (#36) #56

Puterism commented Jul 9, 2021 •

edited

Loading

`Panel`

`Panel.Header`

`Panel.Content`

SunYoungKwon Jul 10, 2021 •

edited

Loading

Puterism commented Jul 12, 2021 •

edited

Loading