-
Notifications
You must be signed in to change notification settings - Fork 4
[Fix] Ticketing View를 로컬 데이터가 아닌 JSON으로 가져온 데이터를 표시하도록 수정 #28
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
Conversation
| @Published var isActivatedWebViewNavigationLink = false | ||
| var ticketingLinkEnabled: Bool { ticketing?.currentTicket?.ticketingURL == nil } | ||
|
|
||
| func onAppear() { |
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.
여기서 사용되는 fetch 메소드들은 @insub4067 의 fetch 코드와 일원화 되면 어떨까하네요 ..!
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.
그러면 NetworkManager 등으로 따로 fetch method를 캡슐화 해줄까요?
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.
고민이 생겼습니다
영이 fetch를 통해 json에서 받아오고자 하는 data 의 type 과 제가 사용하는 data의 type이 다르기 때문에
func 의 return 의 type을 구분하기 위해 거의 같지만 type 만 다른 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.
그건 Protocol, Generic, Async/Await 등을 활용하면 문제가 되지 않을 것 같아요 : )
이 수정은 Optional한 리팩토링 작업이라 지금당장은 우선이 되지않아도 되지않을까 생각되는데 @insub4067 은 어떠실까요 ?
제가 내일 정리해서 이야기드릴까합니다 🙆♂️
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.
json fetch가 한번이 이상 일어나기 때문에 구분하는게 맞다고 봅니다
내일 결론 내어 manager 로 refactoring 진행하는게 좋지 않나 싶습니다
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.
음.. 그냥 return Any로 하는게 답일까 싶은데 내일 얘기 나눠보시죠
| private let shadowColor = Color(.sRGB, red: 0, green: 0, blue: 0, opacity: 0.15) | ||
|
|
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.
이친구는 observed 내부가 아닌 view에 남겨진 이유가 있나요?
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.
해당 값에 의해서 화면이 숨겨지거나 하지 않고, 화면을 그리는 데에만 필요한 값으로 View만 알아야하는 값이라 판단되었습니다
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.
해당 값에 의해서 화면이 숨겨지거나 하지 않고, 화면을 그리는 데에만 필요한 값으로 View만 알아야하는 값이라 판단되었습니다
Color+ Extension으로 빼지 않은 이유는 있나요?
| @Published var isActivatedWebViewNavigationLink = false | ||
| var ticketingLinkEnabled: Bool { ticketing?.currentTicket?.ticketingURL == nil } | ||
|
|
||
| func onAppear() { |
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.
onAppear 라는 함수명이 모호하다고 생각합니다.
onViewAppear 처럼 어떤 객체의 변화인지를 쓰면 좋을 것 같습니다!
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.
- isTicketLinkeEnabled 로 변수명을 수정하는게 어떨까요?
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.
d0ecaec 에서 수정하였습니다 🙆♂️
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.
실제 뷰에서 실행되는 modifier와 통일되어야 동일한 타이밍에 호출된다는 것을 알기 쉽다고 생각되었어요 .. !
onViewAppear 로 되면 View에서 onAppear`와는 별도로 새로운 method가 있다고 생각될 것도 같네요
혹시 needToCallOnAppear와 같은 네이밍은 어떠신가요 ?
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.
view 는 observed(viewModel) 에게 view 가 나타났다 (onAppear 실행됐다) 정도만 알리면 될거 같습니다
따라서 viewDidAppear 정도면 합리적인 네이밍이지 않을까요
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.
viewDidAppear는 UIKit 메소드와 같아서 사견으로 SwiftUI에서는 지양하고 싶네요
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.
SwiftUI의 역습 (?) 🚀
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.
view 떼고 didAppear 어떠세요 그럼?
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.
- readyViewToAppear
- whenViewAppear
작명소 OPEN!
|
머지 되니까 바로 컨플릭트 대잔치군요.. |
E-know
left a comment
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.
고생하셨습니다!
Overview
v1.0의 Ticketing view에서 표시되고 있는 데이터들이 모두 Local image로 지정될 경우, v1.0 다운로드 후 유저가 업데이트 하지 않으면 영원..히… 이 이벤트들로 고정되는게 리스크가 크다.
-> Ticketing Button은 Firebase image 를 가져와서 표시
(+ 캐싱), Upcoming event banner는 JSON 을 fetch 후 표시.Firebase image를 초기 fetch 할 때, 시간이 소요되기 때문에 스켈레톤을 적용한다.
남은 작업