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

[SCRUM-50] ErrorFeature: 네트워크 에러뷰, 서버통신 에러뷰 UI #57

Merged
merged 8 commits into from
Nov 24, 2024

Conversation

Jihyun247
Copy link
Collaborator

[SCRUM-50] FEAT : ErrorFeature & 네트워크 에러뷰, 서버통신 에러뷰 UI

무엇에 관한 PR 인가요? 🙋

두 에러 뷰 UI

어떤 것을 작업하셨나요? 🛠

  • ErrorFeature 모듈 생성
  • 두 에러 뷰 UI
  • Example app

🌱 PR Point

  • 피알 포인트는 아니고 ... 고새 몇달 스유 안했다고 또 살짝 가물가물 해졌네요 , ,

@Jihyun247 Jihyun247 added the enhancement New feature or request label Nov 14, 2024
@Jihyun247 Jihyun247 requested a review from devMinseok November 14, 2024 15:19
@Jihyun247 Jihyun247 self-assigned this Nov 14, 2024
Copy link
Member

@devMinseok devMinseok left a comment

Choose a reason for hiding this comment

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

고생했어!
에러 화면을 띄우는 방식은 AppView(최상단)에서 띄우면 좋을거 같은데 한번 참고해줭

추가로 에러 화면 트리거 방법 의견:

  • AppCore에서 하위 Core를 보면서 특정액션이 발생했을때 띄우기 <- 이 방법은 AppCore가 비대해지고 매번 액션을 연결해야해서 별로라고 생각해
  • AsyncStream을 활용해서 stream을 트래킹하고 있다가 트리거시 AppCore에서 받아서 노출하기 <- 이건 잘 만들어 놓으면 토스트메시지나 알림 다이얼로그도 활용할 수 있을거 같아! (알림을 띄우는 화면에 종속되지 않도록 가능)

Projects/Feature/CatFeature/Project.swift Outdated Show resolved Hide resolved
@devMinseok devMinseok self-requested a review November 19, 2024 13:45
@Jihyun247
Copy link
Collaborator Author

Jihyun247 commented Nov 23, 2024

남은 task

  • ErrorFeature Core 구현 (재시도 버튼, 홈으로 이동, 문의 이동)
  • '이름 수정', '내 고양이 변경' api 에 스트림 연결 ('고양이 불러오기'는 테스트로 연결된 상태)

좀 더 고민해볼 점

  • ErrorFeature에서 재시도 버튼 action 발생 시 이후 처리를 어떻게 해야할지

Comment on lines 52 to 67
.fullScreenCover(isPresented: $store.isLoading) {
VStack {
Spacer()
ZStack {
RoundedRectangle(cornerRadius: Alias.BorderRadius.medium)
.foregroundStyle(Alias.Color.Background.inverse)
.opacity(Global.Opacity._90d)
LottieView(animation: AnimationAsset.lotiSpinner.animation)
.playing(loopMode: .loop)
}
.frame(width: 82, height: 82)
Spacer()
}
.presentationBackground(.clear)
// transaction으로 로딩뷰만 애니메이션을 disable하고싶은데 잘 안됨
}
Copy link
Member

Choose a reason for hiding this comment

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

LoadingView를 따로 분리해서 만들어서 사용하는게 좋을거 같네

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

엉 안그래도 빼야겠다 생각은 했는데 로딩뷰를 대체 어느 모듈에 둬야할지 고민중 @.@
로딩뷰는 걍 Feature 모듈에 두는거 어때

Copy link
Member

Choose a reason for hiding this comment

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

어 그래도 괜찮을거 같아!

Spacer()
}
.presentationBackground(.clear)
// transaction으로 로딩뷰만 애니메이션을 disable하고싶은데 잘 안됨
Copy link
Member

Choose a reason for hiding this comment

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

다른 View 컴포넌트도 같이 애니메이션되어서 그런걸까?
아래 처럼 일단 애니메이션을 없애는 방법도 참고해줘
.animation(nil, value: UUID())

Comment on lines +21 to +22
public var sendServerState: @Sendable (_ state: ServerState) async -> Void
public var updateServerState: @Sendable () -> AsyncStream<ServerState> = { .never }
Copy link
Member

@devMinseok devMinseok Nov 24, 2024

Choose a reason for hiding this comment

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

목적에 종속되지 않도록 만드는게 좋을거 같아
Key(네트워크 에러 리스너)와 value(에러타입) 쌍으로 이루어진 dictionary를 갖게해서 (service locator 구현 하듯이)
send할때 key값과 value를 주고 receive 할때 key값을 넘겨서 그 key의 value만 옵저빙 하도록

좀 더 도움을 주자면... value에 AsyncStream을 가지고 있도록 해서 여러 stream을 지니도록 만들어서 key에 맞는 stream을 받도록 해야될거 같아

Copy link
Member

Choose a reason for hiding this comment

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

이런식으로 해야 앞으로 개선해야하는 토스트 메시지, 네트워크 오프라인 유무 등에도 적용하기 좋을거 같네

devMinseok
devMinseok previously approved these changes Nov 24, 2024
@Jihyun247
Copy link
Collaborator Author

남은 task

  • 네트워크 연결 유실 시 재시도 처리 (현재는 아무 동작 안하고 에러뷰만 dismiss)
  • 서버통신 에러 시 홈으로 이동 -> (온보딩에서 서버통신 에러가 나고 '홈으로 이동'을 누르면 온보딩 첫페이지로 이동해야함 -> 이때 온보딩 carousel 이미지들이 안뜨는 이슈)

리팩토링

  • 민석 코멘트 반영해서 스트림리스너 리팩토링
  • network 연결 유실 시 리퀘스트-리스폰스로 네트워크연결유실을 캐치하는 것은 너무 오래걸림. NetworkTracking 모듈을 사용하는 방법 고민 중 ... ... ..

@Jihyun247 Jihyun247 merged commit 54b4b39 into develop Nov 24, 2024
1 check passed
@Jihyun247 Jihyun247 deleted the feature/SCRUM-50 branch November 24, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants