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

SummaryApi 리팩토링 및 긴글요약 UI 변경 #18

Merged
merged 9 commits into from
Jan 8, 2025
Merged

Conversation

qlrmr111
Copy link
Collaborator

@qlrmr111 qlrmr111 commented Jan 6, 2025

개요

변경 사항

  • 요약 UI 변경에 따른 요약 api 알고리즘 수정
  • 기존 : API 호출 -> 문장요약 -> 전체 문장 받음 -> 그중 랜덤 하나 선택 -> 한줄 요약(content)에 사용
  • 변경 : API 호출 -> 문장요약 -> 전체 문장 받음 -> 받은 전체 문장을 요약 팝업에 리스트로 띄우기 -> 그중하나 선택 -> 한줄 요약에 사용

공유사항(배운 것, 참고하면 좋을 것)

스크린샷

작업후

Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-01-07.at.05.39.10.mp4

전달사항

  • 선택 UI 임시로 만든거라 추가 수정예정입니다 UI 상세수정은 미팅때 시간나면 자세하게 다뤄보죠
  • 이번에도 리팩토링 , 디자인 작업이라 두 작업의 성격이 다른데, 코드가 겹치는 구간이라... 겹치는 구간을 따로 브랜치 만들고 PR 해본적이 없어서 일단은 한꺼번에 했습니다. 리팩토링부분이 적어서 사실상 긴글요약 UI, 알고리즘 변경정도로만 생각해주세용

@qlrmr111 qlrmr111 added 🔨 Refactor 코드 리팩토링 작업을 했어요 ✨ Feat 새로운 기능을 구현했어요 🎨 Design 뷰 디자인 작업을 했어요 labels Jan 6, 2025
@qlrmr111 qlrmr111 requested a review from Eunice0927 January 6, 2025 20:43
This was linked to issues Jan 6, 2025
@qlrmr111 qlrmr111 changed the title Feat/summary api SummaryApi 리팩토링 및 긴글요약 UI 변경 Jan 6, 2025
@trafico-bot trafico-bot bot added the 👀 리뷰 요청 PR이 아직 리뷰되지 않고 있어요 label Jan 6, 2025
@trafico-bot trafico-bot bot added the 🔮 은수 Pull Request Reviews assigned to GitHub User: Eunice0927 label Jan 6, 2025
Copy link
Owner

@Eunice0927 Eunice0927 left a comment

Choose a reason for hiding this comment

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

운기님! 새벽 늦은 시간까지 고생 많으셨습니다.
UI 관련해서 논의해볼 수 있도록 내일 회의 전까지 시간 되신다면 미리 피그마 몇 가지 제안 주시면 좋을 것 같습니다. 저도 시간 되면 만들어보겠습니다!

postieColors.backGroundColor
.ignoresSafeArea()

VStack {
Copy link
Owner

Choose a reason for hiding this comment

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

vstack에 기본 spacing 이 들어있습니다. spacing 0으로 조정 해 주시면 항목간 간격 조절이 더 수월하실 것 같습니다.

@@ -347,6 +352,60 @@ extension AddLetterView {
}
}
}

@ViewBuilder
private var SelectSummaryView: some View {
Copy link
Owner

Choose a reason for hiding this comment

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

해당 뷰 같은 코드가 반복되고 있는 것 같은데, SelectSummaryView를 별도의 파일로 분리하고, SelectSummaryViewModel도 하나로 관리하면 좋을 것 같습니다.
ViewModel의 경우 AddLetterView, EditLetterView, SlowPostboxView의 공통 상위 뷰에서 StateObject로 선언하고
AddLetterView, EditLetterView, SlowPostboxView 내부에는 ObservedObject로 선언한 다음 AddLetterView, EditLetterView, SlowPostboxView를 생성할 때 주입해주는 방식이 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

흠 그럼 SelectSummaryView말고도 다른 반복되는 뷰들도 많은데 그렇게 하는데 더 낫겠죠?

Copy link
Owner

Choose a reason for hiding this comment

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

우선 운기님이 새로 만들어주신 SelectSummaryView만이라도 분리되면 좋을 것 같아요!
다른 뷰도 반복이 많기는 한데 뷰모델을 분리할 방법을 생각을 아직 못해봤습니다!!

.padding(.top)

// summaryList에서 하나를 선택할 수 있는 기능
List(addLetterViewModel.summaryList, id: \.self) { summary in
Copy link
Owner

Choose a reason for hiding this comment

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

리스트 대신 ForEach를 사용하면 어떨까요?
ForEach문 안에서 Text와 구분선용 Rectancle.height(1) 을 생성하면 좋을 것 같습니다.
구분선의 경우 ForEach문의 마지막 index일 때 표시되지 않도록 하면 좋을 것 같습니다.

Comment on lines 197 to 208
func getSummary(isReceived: Bool) async {
do {
let summaryResponse = try await APIClient.shared.postRequestToAPI(
title: isReceived ? "\(sender)에게 받은 편지" : "\(receiver)에게 쓴 편지",
let summaries = try await APIClient.shared.postRequestToAPI(
content: text
)

await MainActor.run {
summary = summaryResponse
showSummaryTextField()
summaryList = summaries
showSelectSummaryView()
}
} catch {
await MainActor.run {
Copy link
Owner

Choose a reason for hiding this comment

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

만약 SelectSummaryViewModel 별도 생성한다면 본 함수도 통채로 함께 이동시키면 될 것 같습니다.
이동하게 될 경우 기존 뷰모델에서 사용되지 않는 import도 챙겨서 제거 해 주세요.

Comment on lines 27 to 30
guard let apiGatewayKey = apiGatewayKey else { return [] }
guard let apiKey = apiKey else { return [] }
guard let requestId = requestId else { return [] }
guard let apiEndpoint = apiUrl else { return [] }
Copy link
Owner

Choose a reason for hiding this comment

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

가드문 하나로 합치면 좋을 것 같습니다!

guard let apiGatewayKey = apiGatewayKey,
      let apiKey = apiKey,
      let requestId = requestId,
      let apiEndpoint = apiUrl else { return [] }

Comment on lines +70 to +73
let summaryList = text
.split(separator: "-")
.map { $0.trimmingCharacters(in: .whitespacesAndNewlines) }
.filter { !$0.isEmpty }
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 76 to 93
// Section("SummaryTest") {
//
// TextField("content", text: $content)
//
// Text(summary)
//
// Button(action: {
// Task {
// do {
// summary = try await APIClient.shared.postRequestToAPI(title: "", content: content)
// } catch {
// summary = "에러 발생"
// Logger.firebase.info("에러 정보: \(error)")
// }
// }}, label: {
// Text("요약하기")
// })
// }
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.

아 이번에 summary라 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 116 to 142

func errorMessage(_ statusCode: Int, errorCode: String) -> String {
switch (statusCode, errorCode) {
case (400, "E001"):
return "빈 문자열 or blank 문자"
case (400, "E002"):
return "UTF-8 인코딩 에러"
case (400, "E003"):
return "문장이 기준치보다 초과했을 경우"
case (400, "E100"):
return "유효한 문장이 부족한 경우"
case (400, "E101"):
return "ko, ja 가 아닌 경우"
case (400, "E102"):
return "general, news 가 아닌 경우"
case (400, "E103"):
return "request body의 json format이 유효하지 않거나 필수 파라미터가 누락된 경우"
case (400, "E415"):
return "content-type 에러"
case (400, "E900"):
return "예외처리가 안된 경우(Bad Request)"
case (500, "E501"):
return "엔드포인트 연결 실패"
case (500, "E900"):
return "예외처리가 안된 오류(Server Error)"
default:
return "알 수 없는 에러"
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.

이번에 API를 바꾸면서 에러처리 부분이 완전 바꼈더라구요 일일이 변경보다 다 지우고 새로 하려 했는데 까먹었나 봅니다.. (이제 생각나네요) 작업 빨리 다시 할게요

Copy link
Owner

Choose a reason for hiding this comment

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

넣어주실때 return의 문구가 유저한테 보일 수 있는 여지가 있다면 조금 더 어휘나 문장이 다듬어져야할 것 같습니다!
유저에게 보이지 않는 것이라면 상관 없습니다!

@trafico-bot trafico-bot bot removed the 🔮 은수 Pull Request Reviews assigned to GitHub User: Eunice0927 label Jan 7, 2025
Copy link
Owner

@Eunice0927 Eunice0927 left a comment

Choose a reason for hiding this comment

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

SelectSummaryView를 반복적으로 또 추가하게 된 점이 아쉽네요.
고생하셨습니다.

@trafico-bot trafico-bot bot added the 👍 Merge 진행 PR 승인이 완료되어 merge할 수 있어요 label Jan 8, 2025
@qlrmr111 qlrmr111 merged commit 31b0e4e into dev Jan 8, 2025
@trafico-bot trafico-bot bot added 👏 Merge 완료 PR이 성공적으로 merge되었어요 and removed 👀 리뷰 요청 PR이 아직 리뷰되지 않고 있어요 👍 Merge 진행 PR 승인이 완료되어 merge할 수 있어요 labels Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 Design 뷰 디자인 작업을 했어요 ✨ Feat 새로운 기능을 구현했어요 👏 Merge 완료 PR이 성공적으로 merge되었어요 🔨 Refactor 코드 리팩토링 작업을 했어요
Projects
None yet
Development

Successfully merging this pull request may close these issues.

긴글요약 UI 변경 SummaryApi 리팩토링
2 participants