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

오버레이 표시 여부를 관리하는 overlay controller 모듈을 추가합니다. #1846

Closed
wants to merge 20 commits into from

Conversation

giwan-dev
Copy link
Contributor

@giwan-dev giwan-dev commented Jan 17, 2022

PR 설명

팝업, 모달, 다이얼로그 등을 오버레이로 총칭하고, 이들 UI의 표시 여부를 관리할 수 있는 overlay controller 모듈을 추가합니다.
history context를 대체하는 것이 목적입니다. history context보다 선언하기 쉽고, 오류가 적은 모델을 목표로 합니다.

사용 방법

먼저 pages/_app.tsx에 Provider를 추가하세요. 다른 context의 의존성이 없기 때문에 순서는 상관 없습니다.

<OverlayControllerProvider>
  {/* ... */}
</OverlayControllerProvider>

그 다음 필요한 컴포넌트에서 훅으로 컨트롤러를 선언해줍니다. 이때, 파라미터로 고유한 id를 넘겨줘야합니다.

const myDialog = useOverlayController('my-dialog')

컴포넌트에 표시 여부, 표시, 비표시 메서드를 연결합니다.

<button onClick={() => myDialog.show()}>다이얼로그 표시</button>

<MyDialog open={myDialog} onCloseClick={() => myDialog.close()} />

변경 내역

  • overlay 패키지 추가
  • overlay controller 모듈 추가
  • 필요한 테스트 코드 추가

history-context와 차이점

  • next.js 의존성을 사용하지 않습니다. 대신 hash에 직접 접근하며, 이벤트 또한 직접 구독합니다.
  • 초기 해시를 참고합니다. 해시가 붙은 채로 랜딩하면 오버레이가 열리게 할 수 있습니다.
  • back 대신 빈 해시로 오버레이를 닫습니다. 예측하기 힘든 back 대신 빈 해시를 히스토리에 추가합니다. 따라서 네비게이션으로 자유롭게 오버레이를 닫을 수 있으면서도 뒤로 가기의 결과를 기다릴 필요 없이 동기 코드를 짤 수 있습니다.

체크리스트

  • 해시 변경할 때 Next.js의 push 사용하기

@giwan-dev giwan-dev requested a review from a team as a code owner January 17, 2022 05:54
@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #1846 (35b34c7) into main (87d1993) will increase coverage by 0.29%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1846      +/-   ##
==========================================
+ Coverage   23.80%   24.10%   +0.29%     
==========================================
  Files         507      510       +3     
  Lines        9900     9941      +41     
  Branches     2963     2967       +4     
==========================================
+ Hits         2357     2396      +39     
- Misses       7257     7259       +2     
  Partials      286      286              
Impacted Files Coverage Δ
packages/overlay/src/controller/index.ts 0.00% <0.00%> (ø)
packages/overlay/src/index.ts 0.00% <0.00%> (ø)
packages/overlay/src/controller/context.tsx 100.00% <100.00%> (ø)

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 87d1993...35b34c7. Read the comment docs.

@giwan-dev giwan-dev added this to the v6.0.0 milestone Jan 17, 2022
@giwan-dev giwan-dev added the help wanted Extra attention is needed label Jan 17, 2022
inbeom
inbeom previously approved these changes Jan 17, 2022
Copy link
Contributor

@inbeom inbeom left a comment

Choose a reason for hiding this comment

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

오 빨리 테스트해보고 싶네요! Next.js의 페이지 라우팅은 어떻게 반응하는지 궁금해요. History provider에서는 iOS와 Android에서의 뒤로가기 동작을 다르게 제어하는 내용도 있는데, 그것에 대한 OverlayController의 처리 방법도요

@giwan-dev
Copy link
Contributor Author

Next.js의 페이지 라우팅은 어떻게 반응하는지 궁금해요.

Next.js의 hashChangeStart, hashChangeComplete 이벤트를 말씀하시는 걸까요? 요거는 한 번 테스트해보겠습니당

History provider에서는 iOS와 Android에서의 뒤로가기 동작을 다르게 제어하는 내용도 있는데, 그것에 대한 OverlayController의 처리 방법도요

요 부분은 제가 배경 설명을 누락했네요!
다른 서비스(토스ㅋㅋ) 레퍼런스를 보니 iOS에서도 시트가 떠 있을 때 뒤로 가기를 하면 시트만 닫히더라구요.
"뒤로 가기를 했을 때 화면을 가린 UI만 닫힌다", "앞으로 가기 하면 닫았던 UI가 다시 보인다" 이 스펙이 iOS, Android 상관 없이 좋은 UX라고 판단해서 분기를 없앴습니다.

@giwan-dev
Copy link
Contributor Author

Next.js의 hashChangeStart, hashChangeComplete 이벤트

요거 작동을 안 하는군요....ㅋㅋㅋ ㅠㅠ
라우팅할 때 Next.js 의존성을 넣을 수 밖에 없겠네요.

@inbeom
Copy link
Contributor

inbeom commented Jan 17, 2022

Next.js Router를 벗어난 곳에서 History를 변경하게 되면 Push 시에나 뒤로가기 시에 원치 않는 일반 HTTP Routing이 일어날 때가 있더라고요. 그런 경험이 생각나서 테스트가 필요하다고 보았어요.

iOS와 Android의 다른 동작은 클라이언트, 기획, 디자인 등등 Stakeholder가 꽤 많아서 협의는 한 번 하고 넘어가야 할 것 같긴 해요. 현재 웹 페이지들의 동작은 각 플랫폼의 Native client 동작과 최대한 유사하게 맞춰 둔 거여서요! 이를테면 iOS의 네이티브 뷰에서 보이는 팝업/모달은 엣지 스와이프를 해도 닫히지 않는데, Android에서는 뒤로가기로 닫을 수 있거든요.

Next.js의 해시 변경 이벤트를 활성화하기 위해 next/router로 해시를 변경합니다.
@giwan-dev
Copy link
Contributor Author

고민해보다보니 iOS, Android 분기와 별개로 계속 push를 하는 게 뒤로가기로 창을 닫는 액션에 영향을 줄 수 있겠네요..ㅠㅠ
좀 더 고민해보겠습니다.

@giwan-dev
Copy link
Contributor Author

UX 논의 결과에 맞게 변경해서 돌아오겠습니다.

@giwan-dev giwan-dev closed this Jan 19, 2022
@giwan-dev giwan-dev removed this from the v6.0.0 milestone Jan 19, 2022
@appear
Copy link
Contributor

appear commented Feb 5, 2022

Next.js의 페이지 라우팅은 어떻게 반응하는지 궁금해요.

Next.js의 hashChangeStart, hashChangeComplete 이벤트를 말씀하시는 걸까요? 요거는 한 번 테스트해보겠습니당

History provider에서는 iOS와 Android에서의 뒤로가기 동작을 다르게 제어하는 내용도 있는데, 그것에 대한 OverlayController의 처리 방법도요

요 부분은 제가 배경 설명을 누락했네요! 다른 서비스(토스ㅋㅋ) 레퍼런스를 보니 iOS에서도 시트가 떠 있을 때 뒤로 가기를 하면 시트만 닫히더라구요. "뒤로 가기를 했을 때 화면을 가린 UI만 닫힌다", "앞으로 가기 하면 닫았던 UI가 다시 보인다" 이 스펙이 iOS, Android 상관 없이 좋은 UX라고 판단해서 분기를 없앴습니다.

토스는 모달, 바텀시트 등등 .. only state 제어에요 (속닥속닥)

@giwan-dev giwan-dev deleted the feature/overlay-controller branch February 23, 2022 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants