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

Feature/moit list #72

Merged
merged 116 commits into from
Jul 27, 2023
Merged

Feature/moit list #72

merged 116 commits into from
Jul 27, 2023

Conversation

chansooo
Copy link
Member

@chansooo chansooo commented Jul 23, 2023

What is this PR? 🔍

  • 이슈 :
  • 관련 문서 :

Changes 📝

  • MOITList에서 데이터 가져오는 로직 구현하였습니다
  • 아직 실제 데이터가 아닌 Mock 구현해서 가져오도록 했습니다.

Screenshot 📸

스크린샷 2023-07-23 오후 7 15 35

To Reviewers 🙏

  • PR너무 커질 것 같아서 중간에 한번 날립니다!
  • 인터랙션과 실제 데이터 받아오는 부분 추가로 구현해서 pr드리도록 하겠습니다...!
  • UI 이상한 부분들 조금 있는데 다듬어서 다시 올릴게요ㅕ=
  • 중간에 develop pull 받는 바람에 다른 커밋이 섞여들어갔는데 feat/moitlist 위주로 봐주시면 될 것 같습니다..!

hope1053 and others added 30 commits June 3, 2023 17:36
두가지 case로 나눠서 각각 tintcolor와 backgroundcolor 설정할 수 있도록 함
…T-iOS into DesignSystem/StudyTableViewCell

# Conflicts:
#	DesignSystem/DemoApp/Sources/DesignSystemViewController.swift
… DesignSystem/StudyTableViewCell

# Conflicts:
#	DesignSystem/DemoApp/Sources/DesignSystemViewController.swift
#	DesignSystem/DemoApp/Sources/NavigationBar/NavigationDemoViewController.swift
#	DesignSystem/Sources/NavigationBar/NavigationItem.swift
#	DesignSystem/Sources/TapPager/MOITSegmentPager.swift
#	DesignSystem/Sources/TapPager/MOITTabPager.swift
@chansooo chansooo self-assigned this Jul 23, 2023
Copy link
Member

@hope1053 hope1053 left a comment

Choose a reason for hiding this comment

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

굿~~~ 수고하셨습니다👍

Copy link
Member

Choose a reason for hiding this comment

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

불필요한 주석은 삭제해주세용🙇‍♀️

@@ -59,7 +59,7 @@ public final class MOITAlarmView: UIView {
public override func layoutSubviews() {
super.layoutSubviews()
self.flexRootView.pin.all()
self.flexRootView.flex.layout()
self.flexRootView.flex.layout(mode: .fitContainer)
Copy link
Member

Choose a reason for hiding this comment

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

layout() 메서드의 mode default값이 .fitContainer라서 굳이 이렇게 작성해줄 필요는 없을 것 같아요~!

Copy link
Member

Choose a reason for hiding this comment

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

요건 그냥 궁금한건데...이런 특정 케이스의 뷰도 MOITList RIB 내부에 들어가는 것보다 Design System에 보통 만드는 편인가요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

저도,,, 잘 모르겠긴 한데 일단 컴포넌트라 분리해두긴 했습니다.
이거는 팀바이팀일 것 같아요


import Foundation

public struct MOIT {
Copy link
Member

Choose a reason for hiding this comment

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

몬가...MOIT보다 MOITListDetail같은 네이밍이 좋을 것 같다는 저스트 의견입니다🤧

Copy link
Member Author

Choose a reason for hiding this comment

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

요거는 조금 더 고민해볼게요!


var fetchLeftTimeUseCase: FetchLeftTimeUseCase { dependency.fetchLeftTimeUseCase }

var fetchPaneltyToBePaiedUSeCase: FetchPenaltyToBePaidUseCase { dependency.fetchPaneltyToBePaiedUSeCase }
Copy link
Member

Choose a reason for hiding this comment

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

USe 대문자 오타인듯합니다👀

Copy link
Collaborator

@SongSeoYoung SongSeoYoung left a comment

Choose a reason for hiding this comment

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

수고수고하셨음니당 ~~ 얼른 합치고싶어요 !! 👍

self.addSubview(flexRootView)
self.flexRootView.pin.all()
self.flexRootView.flex.layout()
self.flexRootView.layer.cornerRadius = 30
Copy link
Collaborator

Choose a reason for hiding this comment

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

clipsToBounds나 masksToBounds 추가하시는거 어떠신가용 ~

Comment on lines +187 to +191
extension MOITListInteractor: MOITListPresentableListener {

func didTapDeleteMOIT(index: Int) {
deleteMoitIndex.accept(index)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오오 ~ 스트림으로 쭉 관리하는군요 !! 👍

Comment on lines 228 to 229
alarmRootView.flex.layout()
flexRootView.flex.layout()
Copy link
Collaborator

Choose a reason for hiding this comment

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

alarmRootView도 layout다시 그리고 flexRootView도 다시 그리는 이유가 있나요 ??
뭔가 뇌피셜이지만 (테스트는 안해봤습니다 ... ㅎㅎ)

pagableAlarmView.flex.markDirty()
alarmRootview.flex.markDirty()
flexRootView.flex.layout()

flexRootView안에 alarmRootView가 있을 것 같고, alarmRootView안에 pagableAlarmView가 있는 것 같아서,
flexRootView를 다시 레이아웃 잡을 때 markDirty된 부분이 다시 레이아웃을 잡지 않을까해서요 ! 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

요거 확인해봤는데 목 데이터로 해서 그런지 몰라도 제안해주신 대로 해도 정상적으로 나옵니다!
근데 flexRootView.flex.layout()만 해줘도 되긴 하네요??? 이건 실제 데이터 받아오는 로직이면 적용 되는지 한번 확인해봐야 할 것 같아요!
@SongSeoYoung


// MARK: UI

private lazy var layout: UICollectionViewFlowLayout = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

잘 몰라서 그러는데 .. 현 상황에서 lazy를 써서 얻는 성능상에 이득이 있나요 ??

Copy link
Member Author

Choose a reason for hiding this comment

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

여기선 빠져도 무방할 것 같아요 ㅋㅋㅋ 아래의 collectionView에서 layout 사용해서 collectionView만 lazy 처리해주면 될 것 같아요

@chansooo chansooo merged commit c6ca235 into develop Jul 27, 2023
1 check failed
@chansooo chansooo deleted the feature/MOITList branch July 27, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants