[Feat/NST-51] #25 Schedule picker 컴포넌트 작성 #29
Conversation
| init(timeHeaders: [String], dateHeaders: [String], mode: Mode) { | ||
| self.timeHeaders = timeHeaders | ||
| self.dateHeaders = dateHeaders | ||
| self.mode = mode | ||
| super.init(frame: .zero, collectionViewLayout: layout) | ||
| self.layout.configure(totalRows: timeHeaders.count + 1, totalColumns: dateHeaders.count + 1) | ||
| self.register(SchedulePickerCell.self, forCellWithReuseIdentifier: SchedulePickerCell.identifier) | ||
| self.cellAvailability = calculateCellAvailability(totalRows: timeHeaders.count + 1, | ||
| totalColumns: dateHeaders.count + 1, | ||
| //Fix: mockMemberStartTime 변경 필요 | ||
| startTimes: mockMemberStartTime) | ||
| } |
There was a problem hiding this comment.
캬 야무지네요 다만 init 아래의 내용은 팀 코드 컨벤션대로 따로 셋업 함수에서 처리해주세요!
| } | ||
|
|
||
| func addSelectedCell(at indexPath: IndexPath) { | ||
| guard mode == .editMode else { return } |
|
|
||
| func addSelectedCell(at indexPath: IndexPath) { | ||
| guard mode == .editMode else { return } | ||
| if let cell = cellForItem(at: indexPath) as? SchedulePickerCell { |
There was a problem hiding this comment.
위에서 guard문으로 체크하는 김에 같이 작성해서
editMode && SchedulePickerCell
조건일때만 아래로 넘기겠다의 문맥이면 더 좋을 것 같네요!
| self.register(SchedulePickerCell.self, forCellWithReuseIdentifier: SchedulePickerCell.identifier) | ||
| self.cellAvailability = calculateCellAvailability(totalRows: timeHeaders.count + 1, | ||
| totalColumns: dateHeaders.count + 1, | ||
| //Fix: mockMemberStartTime 변경 필요 |
There was a problem hiding this comment.
해당 TODO or FIX 사항은 해결된건가요??
There was a problem hiding this comment.
아뇹 ㅋㅋ 이부분은 화면 만들면서 다시 바꿔볼 예정입니다 ..
| cell.isSelectedCell.toggle() | ||
| if cell.isSelectedCell { | ||
| selectedCells.insert(indexPath) | ||
| } else { | ||
| selectedCells.remove(indexPath) | ||
| } |
There was a problem hiding this comment.
너무 절차형같아요 반응형으로 리팩토링하는 고민을 해보는건 어떨까요?
현재 코드
- CV에서 Cell의 내용을 토글
- CV에서 Cell의 내용을 참조해서 CV의 내용을 수정
반응형이라면
- CV와 Cell의 데이터가 바인딩
- cell or cv에서 변경되면 나머지도 같이 변경
There was a problem hiding this comment.
모르게쒀요... 이부분 말고도 전체적인 흐름을 반응형으로 바꾸기 위해서 selectedCells와 cellAvailability를 behaviorRelay로 변경하는게 나을까요
반응형으로 리팩토링해야한다면 변경해야하는 부분은 이 토글부분 말고는 안보이기는 하는데 . . .
| var isSelectedCell: Bool = false { | ||
| didSet { | ||
| backgroundColor = isSelectedCell ? .appBlue400 : .appWhite | ||
| } | ||
| } |
There was a problem hiding this comment.
rx 말고 didSet 사용한 이유가 있을까요?
전 일단 해당 코드에 동의해요! :)
There was a problem hiding this comment.
굳이 rx로 안해도될거같아서 .... didSet을 사용했는데 딱히 이유는 없어요 😅😅
동의하신 이유를 알려주시죠 ㅋ.
There was a problem hiding this comment.
ㅋㅋ 똑같습니다 굳이 rx로 안해도 돼서라고 생각해요!
내-외부에서 구독해야하는 스트림과 일련의 비즈니스 로직이 존재하는 것도 아니고,
오히려 Rx를 위해 Cell 별 DisposeBag과 RxCocoa를 통한 subscribe 로직이 필요할테니까요.
Cell이 특히나 많이 생성될 여지가 있는 부분에서 didSet으로 역할을 간소화시킨건 좋다고 생각해요.
| self.layer.borderColor = UIColor.appGray200.cgColor | ||
| } | ||
|
|
||
| func configureHeader(for indexPath: IndexPath, dateHeaders: [String], timeHeaders: [String]) { |
There was a problem hiding this comment.
코드의 역할이 너무 많은것 같아요! 세부적으로 기능을 쪼개면 더 읽기 편하고 명료한 코드가 될 것 같습니다.
추가로 if 케이스 처리가 여러개라면 Switch를 통해 가독성을 높이는 방법도 고려해보세요!
|
|
||
| import UIKit | ||
|
|
||
| final class SchedulePickerLayout: UICollectionViewFlowLayout { |
There was a problem hiding this comment.
야무지네여 야무져 다만 해당 레이아웃은 SchedulePicker에만 해당되는 레이아웃이니 SchedulePicker extension으로 선언해서 프로퍼티임을 강조하는게 어떨까요?
| func configure(totalRows: Int = 0, totalColumns: Int = 0) { | ||
| self.totalRows = totalRows | ||
| self.totalColumns = totalColumns | ||
| invalidateLayout() |
There was a problem hiding this comment.
레이아웃이 말을 안들어서 추가해놨었는데 빼도 될거같음다
| func dateList(_ dateStrings: [String]) -> [String] { | ||
| let formatter = NSTDateUtility(format: .yyyyMMddTHHmmss) // ISO 8601 형식 | ||
| let displayFormatter = NSTDateUtility(format: .MMddEE) // 출력 형식 | ||
|
|
||
| return dateStrings.compactMap { dateString in | ||
| switch formatter.date(from: dateString) { | ||
| case .success(let date): | ||
| return displayFormatter.string(from: date) | ||
| case .failure(let error): | ||
| print("Failed to parse date \(dateString): \(error.localizedDescription)") | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func timeList(_ startTime: String, _ endTime: String) -> [String] { | ||
| let formatter = NSTDateUtility(format: .yyyyMMddTHHmmss) // ISO 8601 형식 | ||
| var result: [String] = [] | ||
|
|
||
| switch (formatter.date(from: startTime), formatter.date(from: endTime)) { | ||
| case (.success(let start), .success(let end)): | ||
| let calendar = Calendar.current | ||
| var current = start | ||
|
|
||
| while current <= end { | ||
| result.append(NSTDateUtility(format: .HH).string(from: current)) // 출력 형식 | ||
| if let nextHour = calendar.date(byAdding: .hour, value: 1, to: current) { | ||
| current = nextHour | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
| default: | ||
| print("Failed to parse start or end time.") | ||
| return [] | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
해당 함수들은 NSTDateUtility 내용에 포함되면서 self로도 사용 가능하니 NSTDateUtility extension으로 두는게 어떨까요!
There was a problem hiding this comment.
NSTDateUtility 파일 내에 extension으로 따로 빼라는 말인거죠?
🔥 Issue
close #25
🔥 변경된 내용
NSTDateUtility 파일에 스케쥴피커에 필요한 케이스 및 함수 추가했습니다.
🔥 PR Point
🔥 ScreenShot