fix(layout): Manage visibility of sidePanel and sideView at main layout prop #892

aooen · 2022-07-22T09:31:34Z

Summary

사이드 패널과 사이드 뷰의 표시 여부를 사용처에서 관리하기 더 편하게 만듭니다.

Details

사이드 패널과 사이드 뷰의 표시 여부를 현재는 reducer로 구현하고 있습니다. 이에 따라서 사용처에서 path 등의 데이터 기반으로 사이드 패널, 사이드 뷰의 표시를 결정할 수 없고 Effect로 handler를 사용해 관리해야 합니다. 따라서 첫째 렌더링 시점에 표시 여부를 맘대로 설정하기 어려운 문제(LayoutProvider의 initialState를 수동 지정하는 방법이 있겠지만 해당 Provider는 최상위에 존재할 가능성이 있기에 맘대로 접근하기 어렵습니다)가 발생합니다. 이에 따라서 https://github.com/channel-io/ch-desk-web/issues/10399 의 문제가 발생합니다.
또한 사용처에서도 사이드 패널, 사이드 뷰의 표시 여부를 별도로 관리하고 있을 것이기 때문에 https://github.com/channel-io/ch-desk-web/pull/10785 에서 다루는 문제가 발생합니다.
굳이 reducer로 관리해줄 필요 없이, layout main container의 prop을 undefined로 주어 사이드 패널, 사이드 뷰가 존재하지 않음을 알릴 수 있고 그를 바탕으로 레이아웃 스타일을 구성할 수 있습니다. 본 CL입니다.

Browser Compatibility

OS / Engine 호환성을 반드시 확인해주세요.

Windows

Chrome - Blink
Edge - Blink
Firefox - Gecko (Option)

macOS

Chrome - Blink
Edge - Blink
Safari - WebKit
Firefox - Gecko (Option)

References

…으로 관리 BREAKING CHANGE: useSidePanelHandler, useSideViewHandler를 사용할 수 없습니다.

changeset-bot · 2022-07-22T09:31:37Z

🦋 Changeset detected

Latest commit: 350e668

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@channel.io/bezier-react	Patch
bezier-figma-plugin	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2022-07-22T09:35:14Z

Codecov Report

Merging #892 (350e668) into next-v1 (cd4f87c) will decrease coverage by 0.37%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           next-v1     #892      +/-   ##
===========================================
- Coverage    69.01%   68.63%   -0.38%     
===========================================
  Files          205      203       -2     
  Lines         2872     2857      -15     
  Branches       791      792       +1     
===========================================
- Hits          1982     1961      -21     
- Misses         775      783       +8     
+ Partials       115      113       -2

Impacted Files	Coverage Δ
...ct/src/layout/components/HeaderArea/HeaderArea.tsx	`0.00% <0.00%> (ø)`
...s/bezier-react/src/layout/components/Main/Main.tsx	`0.00% <0.00%> (ø)`
...layout/components/Side/SideArea/SideArea.styled.ts	`0.00% <ø> (ø)`
...t/src/layout/components/Side/SideArea/SideArea.tsx	`0.00% <0.00%> (ø)`
...omponents/Side/SideViewContent/SideViewContent.tsx	`0.00% <ø> (ø)`
...ges/bezier-react/src/layout/redux/LayoutActions.ts	`0.00% <ø> (-100.00%)`	⬇️
...ges/bezier-react/src/layout/redux/LayoutReducer.ts	`27.27% <ø> (-6.07%)`	⬇️
...ct/src/layout/stories/LayoutPlayground/Content.tsx	`0.00% <ø> (ø)`
packages/bezier-react/src/layout/LayoutContext.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...ackages/bezier-react/src/layout/LayoutProvider.tsx	`0.00% <0.00%> (-100.00%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd4f87c...350e668. Read the comment docs.

inhibitor1217 · 2022-07-25T01:52:11Z

packages/bezier-react/src/index.ts

-export { default as useSidePanelHandler } from 'Layout/hooks/useSidePanelHandler'
-export { default as useSideViewHandler } from 'Layout/hooks/useSideViewHandler'


breaking change이고, 아마 bezier-react의 레이아웃 시스템은 document system에서도 사용하고 있다 보니 협의를 해보시는 것이 좋을 것 같습니다. 👀

breaking change인건 cz commit convention으로 적어놓았습니다.
document system에서 layout을 쓰는건 처음 알았는데, 코드를 검색해보니까 useSidePanelHandler, useSideViewHandler는 쓰지 않는 것 같습니다.

inhibitor1217 · 2022-07-25T01:58:56Z

packages/bezier-react/src/layout/components/Main/Main.types.ts

+  SidePanelComponent?: React.ComponentType
+  SideViewComponent?: React.ComponentType


컴포넌트 타입을 nullable로 넓히는 것보다, usecase에서 레이아웃 컴포넌트가 null을 렌더링하도록 하는 방향으로 제안합니다

SidePanelComponent, SideViewComponent가 null을 return하라는 뜻인가요?
만약 이런 의미로 말씀해주신거라면 Component가 null을 return하는지 아닌지는 받는 쪽에서 알 수가 없기 때문에 힘들 것 같습니다. 해당 내용을 보고 grid style을 지정해야 하기 때문입니다.

<Main SidePanel={SidePanelRoutes} ... /> function SidePanelRoutes() { if (<condition>) { return null } return ... }

위처럼 각 레이아웃 컴포넌트들이 content를 렌더링할 것인지 스스로 분기하고 있었는데,

<Main SidePanel={<condition> ? SidePanelRoutes : undefined} ... />

현재 인터페이스에서는 condition이 각 레이아웃 컴포넌트를 벗어나 상위 레벨로 올라가는 것이 강제됩니다. 이것도 깔끔하지 않아 보여요

레이아웃 prop을 JSX.Element 타입으로 할 수도 있습니다

prop을 JSX.Element로 주는건 오히려 prop 변경으로 인한 리렌더링이 더 많이 발생할 것 같습니다.
그렇게 장황하게 보이게 하진 않을거고, condition 처리를 따로 분리한 후에 useSidePanelRoutes 형식으로 모듈화 계획입니다.

sungik-choi

reducer로 통제하는 상태와 아닌 상태가 나눠져서 조만간 전체적으로 손보는 편이 좋겠네요

changeset을 추가해주세요(영문)! semantic-release에서 changeset으로 변경하면서, 커밋으로 릴리즈 노트를 자동으로 생성해주지 않기 때문에, 브레이킹 체인지에 대한 내용도 별도로 작성해주셔야 합니다.
PR 제목을 영문으로 변경해주시면 감사하겠습니다 🙏 오픈소스임을 고려해서 PR 제목 등은 영문으로 작성하는 방식으로 맞추려고 해요(#895)

github-actions · 2022-07-26T04:17:44Z

Chromatic Report

🚀 Congratulations! Your build was successful!

Seolhun · 2022-07-27T08:36:55Z

packages/bezier-react/src/layout/stories/LayoutPlayground/LayoutPlayGround.stories.tsx

-          <Content />
+          <Content
+            showSidePanel={showSidePanel}
+            showSideView={showSideView}


context를 쓰는 이유는 의존성을 주입하고 일관되게 만들기 위함인데,

<Main SidePanelComponent={showSidePanel ? SidePanelRoute : undefined} SideViewComponent={showSideView ? SideViewComponent : undefined} // .. > <Content showSidePanel={showSidePanel} showSideView={showSideView} // .. />

서로 독립적인 느낌이 드는것 같습니다.
사용처에서 Loading local state를 component context와 병행해서 썼어도 되었을것 같기도 합니다.

openSidePanel / closeSidePanel 하는 곳과 showSidePanel를 사용할 Main가 다르기 때문에 데이터 흐름을 깔끔하게 하기 위하여 redux나 context로 따로 showSidePanel를 보기 좋게 관리해야 하는게 맞습니다.
여기는 storybook이라 따로 데이터 흐름을 정리하지 않고 prop으로 사용하였습니다.
실제 사용시에는 showSidePanel를 redux 등에서 관리하게 될 것입니다.

aooen added 2 commits July 22, 2022 18:18

fix(layout): sidePanel, sideView의 표시 여부를 reducer가 아닌 main layout prop…

9babd92

…으로 관리 BREAKING CHANGE: useSidePanelHandler, useSideViewHandler를 사용할 수 없습니다.

test(stories): layoutPlayground 적용

0587bfe

aooen added the refactoring Issue or PR related to refactoring with no functional changes label Jul 22, 2022

aooen self-assigned this Jul 22, 2022

aooen requested review from quino0627 and sungik-choi July 22, 2022 09:33

inhibitor1217 reviewed Jul 25, 2022

View reviewed changes

aooen requested a review from inhibitor1217 July 25, 2022 07:19

inhibitor1217 approved these changes Jul 25, 2022

View reviewed changes

sungik-choi approved these changes Jul 25, 2022

View reviewed changes

aooen changed the title ~~fix(layout): sidePanel, sideView의 표시 여부를 reducer가 아닌 main layout prop으로 관리~~ fix(layout): Manage visibility of sidePanel and sideView at main layout prop Jul 26, 2022

chore: add changeset

350e668

Seolhun approved these changes Jul 27, 2022

View reviewed changes

aooen merged commit 3040ea5 into channel-io:next-v1 Jul 27, 2022

github-actions bot mentioned this pull request Jul 27, 2022

ci(changesets): version packages (next-v1) #903

Merged

aooen mentioned this pull request Jul 27, 2022

chore(changeset): reword silver-chefs-raise changeset #904

Merged

github-actions bot mentioned this pull request May 30, 2023

ci(changesets): version packages #1369

Merged

		export { default as useSidePanelHandler } from 'Layout/hooks/useSidePanelHandler'
		export { default as useSideViewHandler } from 'Layout/hooks/useSideViewHandler'

		SidePanelComponent?: React.ComponentType
		SideViewComponent?: React.ComponentType

fix(layout): Manage visibility of sidePanel and sideView at main layout prop #892

fix(layout): Manage visibility of sidePanel and sideView at main layout prop #892

Uh oh!

Conversation

aooen commented Jul 22, 2022

Summary

Details

Browser Compatibility

Windows

macOS

References

Uh oh!

changeset-bot bot commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codecov bot commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

inhibitor1217 Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

aooen Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

inhibitor1217 Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

aooen Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

inhibitor1217 Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aooen Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

sungik-choi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 26, 2022

Chromatic Report

Uh oh!

Seolhun Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

aooen Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Jul 22, 2022 •

edited

Loading

codecov bot commented Jul 22, 2022 •

edited

Loading

inhibitor1217 Jul 25, 2022 •

edited

Loading

sungik-choi left a comment •

edited

Loading

aooen Jul 27, 2022 •

edited

Loading