Skip to content

[Fix] 1차 QA 반영 및 버그 수정#127

Merged
Suyeon9911 merged 6 commits intodevelopfrom
feature/#120-detail-qa
Jan 21, 2023
Merged

[Fix] 1차 QA 반영 및 버그 수정#127
Suyeon9911 merged 6 commits intodevelopfrom
feature/#120-detail-qa

Conversation

@Suyeon9911
Copy link
Copy Markdown
Member

👻 작업한 내용

1차 QA 이슈들 수정 완료 !

  1. 꿈 기록이 일정 길이 이상으로 넘어갈 경우 스크롤 되도록 수정
  2. 감정 태그 변경 사항이 바로 반영되도록 수정
  1. 지난 번 PR에서 언급했던 retain cycle 문제 해결
  2. 지난 번 PR에서 언급했던 중복구독 문제 해결
  • 기디 측에서 별도로 요청한 사항
  1. 다른 뷰에 있다가 홈으로 돌아올 경우, 첫 번째 카드로 리셋

🎤 PR Point

저번 PR에서 남겨주신 코드리뷰 반영하였습니다 !! 어떻게 해결했는지 코멘트 남길게요.

코드를 짰을 당시에 집중을 안했었나봐요.. 엉망진창이네.. 그때 코드리뷰 피드백 남겨주신 것처럼

  1. 한개의 메서드 내부에서 너무 많은 역할을 담당 -> 하나의 메서드가 하나의 역할만을 하도록 분리
  2. HomeEntity의 경우 ViewModel에서 이미 받아오고 있기 때문에 굳이 viewModel의 fetchedDreamRecord에 접근해서 다시 넣어줄 필요가 없었음.
  3. fetch하면서 setAdapter를 해주는 과정에서 중복 구독이 일어나고 있음 : CollectionView의 데이터를 업데이트하기 위해 Adapter를 다시 만들어줄 필요가 없음 -> 넘 어렵게 생각했었네요 ㅎ... 단순히 reloadData로 해결 .. 다시 한번 중복구독이 어떤건지 파악할 수 있었어요.. 구독해주는 코드는 뷰디드로드에서 1번만 !!

📮 관련 이슈

@Suyeon9911 Suyeon9911 added suvera 🥦 담당자 fix 버그를 수정합니다. labels Jan 20, 2023
@Suyeon9911 Suyeon9911 self-assigned this Jan 20, 2023
Copy link
Copy Markdown
Contributor

@L-j-h-c L-j-h-c left a comment

Choose a reason for hiding this comment

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

바쁜 와중에 QA 하느라 고생하셨습니다!

Comment on lines +221 to +227
self.genreStackView.subviews.forEach { (view) in
view.removeFromSuperview()
}

tag.forEach {
genreStackView.addArrangedSubview(DreamGenreTagView(type: .home, genre: $0))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 👍

Comment on lines +102 to 114
subScrollView.snp.makeConstraints {
$0.top.equalTo(titleLabel.snp.bottom).offset(Metric.noteLabelTop)
$0.leading.equalToSuperview()
$0.leading.trailing.bottom.equalToSuperview()
}

contentView.snp.makeConstraints {
$0.edges.equalToSuperview()
$0.width.equalTo(subScrollView.snp.width)
}

noteLabel.snp.makeConstraints {
$0.edges.equalToSuperview()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

scrollView를 이용해 깔끔하게 잡아주셨군요...! textView를 사용하는 방법도 있을 것 같슴당~

Copy link
Copy Markdown
Member

@EunHee-Jeong EunHee-Jeong left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다 LGTM~

Comment on lines +249 to +262
private func resetHomeLayoutIfNotEmpty() {
dreamCardCollectionView.setContentOffset(CGPoint(x: 0, y: 0), animated: true)

[self.desciptionLabel, self.dreamCardCollectionView].forEach {
$0.isHidden = false
}

welcomeLabel.snp.remakeConstraints {
$0.top.equalTo(logoView.snp.bottom).offset(Metric.mainLabelTop)
$0.leading.equalToSuperview().inset(Metric.mainLabelLeading)
}

self.emptyLabel.removeFromSuperview()
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

깔끔하다!!

Comment on lines -234 to +206
self.setCollectionViewAdapter()
resetHomeLayoutIfNotEmpty()
self.dreamCardCollectionView.reloadData()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

메서드 분리 굿굿!

@Suyeon9911 Suyeon9911 merged commit 8b0da75 into develop Jan 21, 2023
@Suyeon9911 Suyeon9911 deleted the feature/#120-detail-qa branch January 21, 2023 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix 버그를 수정합니다. suvera 🥦 담당자

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Fix] 중복 구독 및 Retain cycle 문제 해결 [Fix] 상세보기 뷰 오류 수정 + QA 반영

3 participants