-
Notifications
You must be signed in to change notification settings - Fork 15
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
[Feat] #137- 출석조회하기 레이아웃 구현 #152
The head ref may contain hidden characters: "feat/#137-\uCD9C\uC11D\uC870\uD68C\uD558\uAE30-\uB808\uC774\uC544\uC6C3"
Conversation
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.
뷰 깔꼼하게 잘 나누셨네요 . . (최고)
생각보다 볼 코드가 많았네요 . .
너무 고생했서요 . . ♡
앞으로도 하나둘셋 ! 화이팅 !
private let attendanceStateButton: UIButton = { | ||
var configuration = UIButton.Configuration.plain() | ||
configuration.title = "" |
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.
음 버튼말고 imageView + label이 더 나을 것 같아요 . . !
여기 클릭 이벤트가 따로 없는 걸로 이해했는데 맞을까요 ?
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.
네 이벤트 없어요!
그냥 configuration 안써봐서 써봤어용 !! ㅎㅎ
근데 밑에 폰트를 다시 지정해줘야돼서 영인님 리뷰 반영해서 바꿀게용 감사 ! 😊
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.
반영 커밋 : 11d88b5
private func updateScoreTypeLabel(_ type: AttendanceStateType) { | ||
let typeToString = [ | ||
AttendanceStateType.all: I18N.Attendance.all, | ||
AttendanceStateType.attendance: I18N.Attendance.attendance, | ||
AttendanceStateType.tardy: I18N.Attendance.tardy, | ||
AttendanceStateType.absent: I18N.Attendance.absent | ||
] | ||
|
||
if let typeString = typeToString[type] { | ||
singleScoreTitleLabel.text = typeString | ||
} | ||
} |
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.
매번 배열 생성하지 않고 enum 내부에 변수둬서 구현할 수도 있을 것 같아요 ~!
public enum AttendanceStateType {
case all
case attendance
case tardy
case absent
var title: String {
switch self {
case .all:
return I18N.Attendance.all
case .attendance:
return ""
case .tardy:
return ""
case .absent:
return ""
}
}
}
private func updateScoreTypeLabel(_ type: AttendanceStateType) {
singleScoreTitleLabel.text = type.title
}
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.
코드리뷰 반영하여 CaseIterable 프로토콜을 채택하여, AttendanceStateType의 모든 경우를 순회할 수 있도록 했어요.
그리고 rawValue를 이용하여 title 에 넣어줬어요. I18N은 지정 못해서 하드코딩으로 넣었습니당..~~
반영 커밋: e736e49
private func setLayout() { | ||
|
||
addSubviews(myInfoContainerView, myScoreContainerStackView, myAttendanceStateContainerView) | ||
myScoreContainerStackView.addSubviews(allScoreView, attendanceScoreView, tardyScoreView, absentScoreView) |
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.
StackView에 이미 해당 뷰들 추가해준 상태라 이 코드는 지워야될 것 같아요 ~ !
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.
반영 커밋: 3f34f99
cell.setData(title: "\(indexPath.row+1)차 세미나", | ||
image: DSKitAsset.Assets.opStateAttendance.image, | ||
date: "4월 \(indexPath.row*7+1)일") |
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.
얘는 아마 타입 넘겨서 해당 이미지 띄우는 로직으로 수정되겠죠 . . ?
앞서 생성한 AttendanceStateType에 이미지 변수도 추가하면 좋을 것 같아요 ~!
public enum AttendanceStateType {
// 어쩌구 ~
var image: UIImage {
switch self {
case .attendance:
return DSKitAsset.Assets.opStateAttendance.image
// 나머지 케이스 구현
}
}
}
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.
@frozen | ||
public enum OPNaviType { | ||
case oneLeftButton /// 좌측 뒤로가기 버튼 | ||
case oneRightButton /// 우측 버튼 |
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.
이거 oneRightButton은 오른쪽에만 버튼이 있는 경우인가요 ??
이런 뷰가 있었나 해서요 . . 몰라서 . .
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.
이런 뷰 없었어요 ㅋㅋㅋ
근데 현재 양쪽에 버튼이 있어서 그냥 만드는김에 왼쪽, 오른쪽에 버튼 하나짜리는 언젠가 쓰이겠지 하고 같이 만들었어요 ㅎㅎ
오른쪽 버튼은 나중에 dismiss할때 쓰면 될거같아요 👍🏻
private lazy var todayInfoStackView: UIStackView = { | ||
let stackView = UIStackView(arrangedSubviews: [dateAndPlaceStackView, titleLabel]) | ||
stackView.axis = .vertical | ||
stackView.spacing = 16 | ||
stackView.alignment = .leading | ||
return stackView | ||
}() |
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.
이 코드부터 stackView 하나만 사용하는 방향으로 바꾸면 좋을 것 같아요 !
아마 spacing이 달라서 스택뷰를 더 생성한 것 같은데 (맞나요 ?)
stackView.setCustomSpacing(16, after: dateAndPlaceStackView)
setCustomSpacing를 통해 스택뷰 내부 간격 조정할 수 있습니다.
밑에 subtitle까지 넣어주면 될 것 같아요 ~ !
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.
일정 없는 날에 todayInfoStackView
는 한 번에 히든하기 위해 컨테이너 안에 한 번 더 묶어놨어요~!
반영 커밋: 883b937
public enum APIType { | ||
case attendance | ||
case auth | ||
case mission | ||
case rank |
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.
얘도 마찬가지로 enum 내부 변수 하나 두고, 같은 base를 쓰게 하는 방법도 있는데
음 . . 어떤 방식이 더 좋을지는 저도 잘 모르겠네요 . . 앱 팀과도 함께 고민을 해도 좋을 듯요 . .
public enum APIType {
case attendance
// 나머지 케이스
var base: String {
switch self {
case .attendance:
return Config.Network.operationBaseURL
case .auth, // 나머지 케이스:
return Config.Network.baseURL
}
}
}
extension BaseAPI {
public var baseURL: URL {
var base = Self.apiType.base
// 생략
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.
이건 앱팀이랑도 같이 상의해보면 좋을거같아요!!
일단 최대한 영향 안주는 방향으로 했는데 기존 솝탬프코드는 네이밍이 default한데 운영서비스만 operation 붙여서 넣어놨어요
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.
영인이가 제시한 방식 좋은 것 같아요!!
private func setLayout() { | ||
view.addSubviews(navibar, containerScrollView) | ||
|
||
containerScrollView.addSubviews(headerScheduleView, attendanceScoreView) |
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.
스크롤 이슈는 scrollView에 직접 컴포넌트들을 추가해서 스크롤 가능한 범위를 제대로 계산하지 못하는 것 같아요 . . !
content view를 생성해서 scrollView에 추가하고, content 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.
요거떄문은 아닌거같습니당 ㅠㅠ
제가 좀더 알아볼게요!!!! 의견 감삼당 🫶🏻
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.
수고하셨습니다~~!!
🌴 PR 요약
🌱 작업한 브랜치
🌱 PR Point
📌 참고 사항
- Config파일 변경된건 카톡을 봐주세요 ! (앱팀 base url 변경 및 운영 서비스팀 base url 추가)
📸 스크린샷
📮 관련 이슈