APP-10298 : PanModalNavigationController 추가 #10
Conversation
There was a problem hiding this comment.
[PanModalNavigationController를 PanModal 안으로 옮기게된 이유]
- extension으로 정의한 PanModalPresentable 디폴트 값이 PanModal 라이브러리 내부와 ZigZag내부에 둘다 정의되어있음
- ZigZag에서 사용할 경우에는 ZigZag내부에 정의된 디폴트 값으로 덮어씌워지므로 상관 없음
- 피처모듈에서 사용할 경우(앱 인터페이스에 PanModalNavigation 정의한 경우) 두 디폴트값중 어느 값을 사용해야할지 알 수 없다는 오류가 발생
- PanModal을 사용한 네비게이션 뷰컨이므로 PanModal 내부에 있는것이 더 맞다는 생각이 듦
There was a problem hiding this comment.
[9월 25일 오프라인 피드백 내용]
- 팔레트가 라이브러리까지 진출하는 것은 영향범위가 너무 넓기 때문에 컬러값을 외부에서 주입받도록 하는것이 어떨까?
[팔레트 컬러 사용하는 곳]
- PanModalNavigationController.dragIndicatorBackgroundColor
- PanModalNavigationController.backgroundColor
- PanModalNavigationController.navigationBar.backgroundColor
- PanModalNavigationController.borderCoordinator.borderView.backgroundColor
[피드백 이후 생각]
- PanModalNavigationController 내부에서 사용하는 borderCoordinator 의 컬러값까지 외부에서 주입받는게 괜찮은가,,?
- 디테일한 부분까지 컬러를 받아야 해서 지그재그에서 사용할 때 오히려 귀찮아지지 않을까 생각이 들었습니다.
- 추후에 혹시 컬러 변경 작업이 컬러 이름에 대한 속성이 변경되는 것보다는 바텀 시트지면에 대해 별도로 진행될 것 같다는 예상이라 라이브러리 내부에 별도로 정의해두는 식으로 가보는게 어떨지 제안드려봅니다 🙇♂️
- 추가로 걱정되는 부분이 있다면 언제든 코멘트 부탁드립니다.
There was a problem hiding this comment.
개인적으로는 color를 외부에서도 설정할 수 있게 열어주고
외부에서 주입한 컬러가 없다면 아래 작성하신 것 처럼 내부의 기본 컬러를 사용하는 방향으로 해줘도 괜찮을것 같습니다
ex)
public func setupAppearance(appearance: PanModalAppearance = .default) { ... }
struct PanModalAppearance {
let aColor: UIColor
...
static let default: Self = ...
}
There was a problem hiding this comment.
[9월 25일 오프라인 피드백 내용]
- 팔레트가 라이브러리까지 진출하는 것은 영향범위가 너무 넓기 때문에 컬러값을 외부에서 주입받도록 하는것이 어떨까?
[팔레트 컬러 사용하는 곳]
- PanModalNavigationController.dragIndicatorBackgroundColor
- PanModalNavigationController.backgroundColor
- PanModalNavigationController.navigationBar.backgroundColor
- PanModalNavigationController.borderCoordinator.borderView.backgroundColor
[피드백 이후 생각]
- PanModalNavigationController 내부에서 사용하는 borderCoordinator 의 컬러값까지 외부에서 주입받는게 괜찮은가,,?
- 디테일한 부분까지 컬러를 받아야 해서 지그재그에서 사용할 때 오히려 귀찮아지지 않을까 생각이 들었습니다.
- 추후에 혹시 컬러 변경 작업이 컬러 이름에 대한 속성이 변경되는 것보다는 바텀 시트지면에 대해 별도로 진행될 것 같다는 예상이라 라이브러리 내부에 별도로 정의해두는 식으로 가보는게 어떨지 제안드려봅니다 🙇♂️
- 추가로 걱정되는 부분이 있다면 언제든 코멘트 부탁드립니다.
- 파일 내 변경점 없음 - PanModalNavigationController내에서 사용하는 BorderCoordinator 파일도 복사
- PanModal 내부에서 사용하는 컬러값만 중복 정의
Raymond-Sik
force-pushed
the
feature/APP-10298-talk-lounge
branch
from
4f786df to
b6fe7cd
Compare
| UIColor.black.withAlphaComponent(0.7) | ||
| } | ||
|
|
||
| public var dragIndicatorBackgroundColor: UIColor { .Palette.surface } |
There was a problem hiding this comment.
(Q)
public var dragIndicatorBackgroundColor: UIColor {
mTopPanModalPresentable?.dragIndicatorBackgroundColor ?? .Palette.surface
}
�컬러를 주입받은 새로운 인터페이스는 없더라도, 이미 있는 코드는 동적으로 열려있어야하지 않을까여??
There was a problem hiding this comment.
요거 기존의 PanModalNavigationController에도 mTopPanModalPresentable?.dragIndicatorBackgroundColor 값과 상관없이 고정값으로 설정되어있었더라구요..!
PanModalPresentable+Defaults에서 기본값이 .clear로 되어있어서 아마 사용하는 곳에서 모두 컬러값을 변경해줘야할 것 같습니다!
말씀주신대로 인터페이스는 열려있는게 맞을 것 같아서 변경해보겠습니다! 👍
There was a problem hiding this comment.
넵넵 원래 고정이었어서, 당장 수정이 필요한 일은 없을꺼 같아요!!
혹시 범위가 너무 넓다면, 다음 스텝으로 가져가도 좋습니다!!
There was a problem hiding this comment.
피처 모듈 분리만으로도 범위가 많이 큰 것 같아서 백로그 티켓으로 하나 생성해두었습니다!
톡라운지 피처모듈이 RT 탑승한다면 후속으로 진행해 보겠습니다!
| view.backgroundColor = .Palette.surface | ||
| navigationBar.isTranslucent = false | ||
| navigationBar.backgroundColor = .Palette.surface |
There was a problem hiding this comment.
-
Border Color 까지 주입 받아야할까?
-> 이부분은 저도 동의합니다!! 불필요해 보이기도 합니다. -
.Palette.surface 의 고유한 RGB 값이 변경되는 경우, 이런 경우 코드상 눈에 잘 안 보일꺼같습니다
앱에서 주입받은 컬러 우선 적용 -> 없는 경우, PanModal 내 컬러 적용
view.backgroundColor = dragIndicatorBackgroundColor
navigationBar.backgroundColor = dragIndicatorBackgroundColor
요약
변경사항