-
Notifications
You must be signed in to change notification settings - Fork 149
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
만국박람회 [Step 2] Gray, Danny #310
base: ic_11_danny
Are you sure you want to change the base?
Conversation
…ViewController로 화면 이동 기능 추가
…ntroller 타입 내부에 한국출품작 디테일 설명 로직 추가, Navigation Controller 타이틀 추가
…, StackView, Maintitle, Subtitle 추가"
…ageTableViewCell 내부의 extension 타입을 활용한 코드 리팩토링
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.
→ 이러한 방식으로 extension 타입을 활용한 코드 정리를 해도 되는 것인지? 별도 규칙이 있는 것인 지 고민 되었습니다. 🤔
요건 취향의 영역이라 편하신대로 하심 됩니다!
→ TableView에 있는 Cell을 재사용하기 위해 아래와 같이 구현하고, 원하는 셀 모앙을 위해 HeritageTableViewCell을 생성하여 활용했습니다.
현재 prepareForReuse가 빠져있어요
cell이 언제 어떻게 재사용되는지, 화면에 몇개가 그려지고 어느 시점에 생성되는지 학습해서 알려주세요!
저희의 의도는 extension 하나로 싶은데, 어떻게 하면 좋을 지 조언을 주시면 감사하겠습니다.
extension이 아닌 AssetDecoder과 같은 객체를 만들어보는것은 어떨까요?
SRP를 생각해보시면 좋을듯 합니다!
→ 스토리보드를 이용하지 않고 코드로 진행하는 현업 조직일 경우에는 스토리보드를 통해 제약조건을 확인하는 것이 아닌 바로 코드를 통해 제약조건을 작성을 하고 계신것 인지 궁금합니다. 😅
개인적으로 스토리보드는 deprecated됐다고 생각합니다.
현업에서 아직 스보를 사용하는 곳은 있겠지만, 코드로 UI를 그릴줄 알면 스토리보드는 볼 수 있어서 코드로 UI를 작성해주세요.
|
||
import UIKit | ||
|
||
class ExpositionViewController: UIViewController { |
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.
class ExpositionViewController: UIViewController { | |
final class ExpositionViewController: UIViewController { |
static dispatch를 위해서 final은 디폴트로 붙여주세용
import UIKit | ||
|
||
class ExpositionViewController: UIViewController { | ||
var expositionIntroduction: ExpositionIntroduction! |
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 var mainTitleLabel: UILabel = { | ||
let label = UILabel() | ||
label.font = UIFont.preferredFont(forTextStyle: .title1) | ||
return label | ||
}() | ||
|
||
private var subTitleLabel: UILabel = { | ||
let label = UILabel() | ||
label.font = UIFont.preferredFont(forTextStyle: .title2) | ||
return label | ||
}() | ||
|
||
private var posterImageView: UIImageView = { | ||
let imageView = UIImageView() | ||
return imageView | ||
}() | ||
|
||
private var visitorLabel: UILabel = { | ||
let label = UILabel() | ||
return label | ||
}() | ||
|
||
private var locationLabel: UILabel = { | ||
let label = UILabel() | ||
return label | ||
}() | ||
|
||
private var durationLabel: UILabel = { | ||
let label = UILabel() | ||
return label | ||
}() | ||
|
||
private var descriptionLabel: UILabel = { | ||
let label = UILabel() | ||
return 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.
let이어도 되지 않을까요?
- UI 프로퍼티는 앵간하면 translatesAutoresizingMaskIntoConstraints = false를 초기화 시점에 해주면 좋아요
private var posterImageView: UIImageView = { | ||
let imageView = UIImageView() | ||
return imageView | ||
}() |
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.
따로 무언가 속성을 바꾸지 않는다면, 클로저로 initialize안하고 바로 해도 괜찮을거 같아요!
상황에 따라~
do { | ||
self.expositionIntroduction = try jsonDecoder.decode(ExpositionIntroduction.self, from: dataAsset.data) | ||
} catch { | ||
print(error.localizedDescription) |
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.
에러처리도 요구사항중에 중요한 부분이라고 생각되는데요,
alert를 표시하던가, 에러 화면을 보여줘야 할거 같아요
private var itemTitle: String? | ||
private var itemImageName: String? | ||
private var itemDescription: String? |
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.
initialize 시점에 주입받으면 좋을 거 같아요
이 정보들이 없으면 detail뷰컨으로 진입못하도록 해야할것 같습니다
guard let unwrappedItemImageName = itemImageName else { | ||
return | ||
} |
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.
guard let unwrappedItemImageName = itemImageName else { | |
return | |
} | |
guard let itemImageName else { return } |
return tableView | ||
}() | ||
|
||
private let cellIdentifier: String = "HeritageTableViewCell" |
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.
https://kirkim.github.io/ios/2022/08/11/cellreuseidentifier.html
요런거 참고해서 protocol로 사용하심 좋을듯
또한 cell의 식별자는 셀 내부에 있음 좋을 거 같아요
cell.useTitleLabel(titleLabelText: koreanHeritage[indexPath.row].name) | ||
cell.useShortDescriptionLabel(shortDescriptionText: koreanHeritage[indexPath.row].shortDescription) | ||
cell.useitemImageView(itemImageName: koreanHeritage[indexPath.row].imageName) |
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.
- koreanHeritage[indexPath.row]가 반복되고 있어요. 위에서 지역변수로 잡는게 좋을 거 같아용
- https://minsone.github.io/programming/check-index-of-array-in-swift 이렇게 안전하게 접근해주세요
cell.configure(with: heritage)
로 한 번에 주입해주시면 좋을 듯용
} | ||
|
||
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell { | ||
let cell: HeritageTableViewCell = tableView.dequeueReusableCell(withIdentifier: self.cellIdentifier, for: indexPath) as! HeritageTableViewCell |
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.
가드문을 활용해주세요! 강제 캐스팅은 없는 기능입니다 (2)
안녕하세요.! @havilog
Gray, Danny 조 입니다.
[STEP 2]PR 요청드립니다. 🙏
🇰🇷 만국박람회 [STEP 2: 화면 구현] 진행사항
- 첫 화면에는 1900 만국박람회 포스터와 각종 설명을 표시합니다.
- 출품작 목록에는 아래의 항목이 포함된 리스트를 출력합니다.
- 출품작 상세화면에는 아래의 항목을 보여줍니다.
🤔 고민되었던 점
extension 타입을 활용해서 기능별로 코드를 분리 하였습니다. (ex. addView 메소드와 setConstraint 하는 메소드를 별도 extension을 활용해서 구현 등.)
→ 이러한 방식으로 extension 타입을 활용한 코드 정리를 해도 되는 것인지? 별도 규칙이 있는 것인 지 고민 되었습니다. 🤔
출품작 목록 화면을 customCell, TableView를 활용하여 구현
→ TableView에 있는 Cell을 재사용하기 위해 아래와 같이 구현하고, 원하는 셀 모앙을 위해 HeritageTableViewCell을 생성하여 활용했습니다.
cell 내부를 구성하는 컴포넌트와 제약조건은 HeritageTableViewCell에 정의하고, TableView의 생성과 제약조건은 KoreanHeritageViewController에 정의하였는데 올바르게 코드를 분리 하였는지 고민 되었습니다.
🙏 조언을 얻고 싶었던 점
: 아래 코드처럼 ExpositionViewController, KoreanHeritageViewController에서 JSON decode의 기능을 구현했습니다. 저희의 의도는 extension 하나로 싶은데, 어떻게 하면 좋을 지 조언을 주시면 감사하겠습니다.
→ 스토리보드를 이용하지 않고 코드로 진행하는 현업 조직일 경우에는 스토리보드를 통해 제약조건을 확인하는 것이 아닌 바로 코드를 통해 제약조건을 작성을 하고 계신것 인지 궁금합니다. 😅
만국박람회 프로젝트 [STEP 2] PR 마치도록 하겠습니다. 감사합니다. 😁