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

[iOS- Dumba]/149/issue detail #151

Merged
merged 4 commits into from
Jun 30, 2021

Conversation

ghis22130
Copy link
Collaborator

No description provided.

@Lia316 Lia316 added enhancement 기존에 있는 기능을 개선 iOS iOS labels Jun 29, 2021
@Lia316 Lia316 added this to In progress in [iOS] 프로젝트 via automation Jun 29, 2021
Copy link
Owner

@Lia316 Lia316 left a comment

Choose a reason for hiding this comment

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

이슈 디테일을 살리시느라 수고하셨습니다~

코멘트랑 질문 달았는데요,
코멘트 반영은 선택사항으로 봐주시면 될 것 같아요 ㅎㅎ

Comment on lines 36 to 41

func fillUI(_ emoji: Emoji) {
text = emoji.value + " " + String(emoji.count)
configureConstraint()
}

Copy link
Owner

Choose a reason for hiding this comment

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

text = emoji.value + " " + String(emoji.count)

Swift에서는 String을 합칠 때, 문자열 보간법을 주로 권고한다고 들어서 정확한 정보를 위해 한번 찾아봤습니다.

두 사이트 모두 실험적으로 어떤 방식이 더 빠른 지 검증해본 결과를 보여주는데요,
이상하게 결과가 다르긴 합니다 😅

제가 얻은 결론은

  • 연산해야할 String이 많아지면 두 방식의 차이가 생긴다 (둘 중에 뭐가 더 우세한지는 결과가 반대여서 헷갈리네요)
  • 문자열 보간법이 코드 가독성에는 더 좋다

이 두 가지로, 참고만 하시면 좋을 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오호 두 상황이 차이가 나는군요! 보간법으로 수정해보겠습니다.

Comment on lines 112 to 117

extension IssueDetailViewController: UITableViewDelegate {
func tableView(_ tableView: UITableView, heightForRowAt indexPath: IndexPath) -> CGFloat {
return 100
return 200
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

이 부분은 CollectionView로 바꿀 거라 상관 없어질 것 같긴 하지만 일단 코멘트 남깁니다!

셀의 높이가 가변적이라, automaticDimension을 사용하여 테이블뷰 셀의 높이를 동적으로 지정하면 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 이부분은 수정 예정입니다!

Comment on lines +1 to +7
{
"images" : [
{
"filename" : "LoadFail.jpg",
"idiom" : "universal",
"scale" : "1x"
},
Copy link
Owner

Choose a reason for hiding this comment

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

로드를 실패할 경우, 따로 처리해주는 이미지를 추가하신 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 맞습니다! 저번에 리뷰어한테 실패시 빈이미지를 출력하는게 옳바른지에 대해서 리뷰를 받았어서
LoadFail

로드 실패시 이 이미지가 출력되게끔 처리했스빈다.

Copy link
Owner

Choose a reason for hiding this comment

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

오홍 사용자 경험에 있어서 훨씬 좋군요! 설명 감사합니다

Comment on lines 14 to 20
extension DateManagable {
static var issueFormatter: DateFormatter {
let formatter = DateFormatter()
formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ss.SSS"
formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"
formatter.locale = Locale(identifier: "ko")
return formatter
}
Copy link
Owner

Choose a reason for hiding this comment

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

formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"

잘 몰라서 질문드립니다!
T 와, Z의 역할이 궁금합니다 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ISO 8601 국제 표준에 의한 데이터 형식입니다!
T는 시간의 시작을 알리는것 같고 Z는 UTC 시간 이용을 알리는 zone의 의미 인것 같습니다.
swift에서는 해당 형식의 데이터를 formatter 하기 위해 'T', 'Z'로 한번 감싸주어야 합니다.

Copy link
Owner

Choose a reason for hiding this comment

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

아하 그럼 이전 코드에서는 format이 제대로 안 되었나요??

Comment on lines 11 to 23
extension UIImageView {
func load(url: String?) {
let url = URL(string: url!)
self.kf.setImage(with: url) { result in
switch result {
case .success(_):
break
case .failure(_):
self.image = UIImage(named: "LoadFail.jpg")
}
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

extension으로 메소드를 작성하니 매번 이미지 처리를 위한 메소드를 작성할 필요가 없어서 너무 좋은 것 같습니다! 😆
편리하고 좋은 기능 추가 감사합니다!

추가로, extension에 해당되는 줄간격 조정 부탁드립니다 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

extension 줄간격 조정하겠습니다 ㅎ

@ghis22130
Copy link
Collaborator Author

리뷰 반영 후 코멘트 남깁니다.
우선 당장 할 수 있는 extension 개행처리, emoji label 보간법 변경 진행 하였습니다.
확인 후 머지 부탁드립니다 ^__^

@Lia316
Copy link
Owner

Lia316 commented Jun 30, 2021

ㅎ__ㅎ 수고 많으셨습니다!

@Lia316 Lia316 merged commit f513a1f into Lia316:iOS/149/IssueDetail Jun 30, 2021
[iOS] 프로젝트 automation moved this from In progress to Done Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 기존에 있는 기능을 개선 iOS iOS
Projects
Development

Successfully merging this pull request may close these issues.

[iOS] 상세 이슈 - Collection View, Diffable 적용 [iOS] IssueDetail - 이슈 상세 페이지
2 participants