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

[#72]메인뷰 데일리 텍스트를 당일 테스크 수행여부에 따라 변경 #77

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

hyeonheebee
Copy link
Collaborator

📝 작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요

  • 메인뷰 데일리 텍스트를 당일 테스크 수행여부에 따라 변경했습니다
  • WeeklyReadingProgressView를 수정했습니다

스크린샷 (선택)

💬 리뷰 요구사항(선택)

의도한 수정사항이 맞는지 확인부탁드려요
코드가 헷갈리지 않는지 확인 부탁드립니다!

@hyeonheebee hyeonheebee self-assigned this Nov 18, 2024
@hyeonheebee hyeonheebee linked an issue Nov 18, 2024 that may be closed by this pull request
3 tasks
Copy link
Collaborator

@zaehorang zaehorang 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 15 to 23
//추가 필요데이터
let daysOfWeek = ["일", "월", "화", "수", "목", "금", "토"]

let today = Date()

var body: some View {
// 추가

let todayIndex = Calendar.current.component(.weekday, from: today) - 1
Copy link
Collaborator

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.

오 이건 제가 주석을 수정했는데 반영이 안된거 같아요 +_+ 커밋 확인 해놓을게요

Comment on lines 30 to 44
// TODO: 텍스트 변경
ForEach(0..<daysOfWeek.count, id: \.self) { index in
if let record = weeklyRecords[index] { // 페이지 할당이 되어있다면
let hasCompletedToday = record.pagesRead == record.targetPages
if index == todayIndex { // today
Text(hasCompletedToday ? "오늘도 성공이에요! 화이팅🤩" : "오늘은 \(record.targetPages)쪽 까지 읽어야해요!")
.font(.system(size: 17, weight: .semibold))
.foregroundColor(.black)
/* Text("\(currentReadingBook.nonZeroReadingDaysCount())일 째 도전중!")
.font(.system(size: 17, weight: .semibold))
.foregroundColor(.black)
*/
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

당일의 결과만 필요한 로직으로 보입니다.

WeeklyPageCalendarView와 비슷하게 구현하신 것으로 보이는데, 제가 그렇게 말씀드려서 참고하신 것 같아요. 😅
하지만, 해당 화면에서는 일주일의 데이터를 모두 가져올 필요 없이 당일의 결과만 확인하면 됩니다.

getWeeklyRecordedPages 메서드를 사용하지 말고, today를 키 값으로 하여 당일의 결과만 가져오도록 변경하면 더 간단해질 것 같아요.
또한, hasCompletedToday 프로퍼티를 활용해 판단하면 충분할 것 같습니다.

요약

  • 일주일 데이터를 가져올 필요 없음: 현재 화면은 특정 날짜(당일) 결과만 확인하면 됩니다.
  • today 키 값으로 필요한 데이터만 가져오고, 간단히 확인하도록 변경해주세요!
  • (+) 지워야 하는 코드는 주석처리 하지 않고 지워도 됩니다! 깃헙에서 친절히 다 보여줍니다 🐯

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 확인하고 수정하겠습니다!

Copy link
Collaborator Author

@hyeonheebee hyeonheebee Nov 19, 2024

Choose a reason for hiding this comment

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

확인해보니 위의 코멘트에 제 답글처럼 제가 마지막 수정사항을 커밋만하고 푸쉬가 안해서 추가작업한 코드, 지워야 하는 코드 주석처리등이 반영이 안된것같아요! 다시 제대로 푸쉬하겠습니다 :)
코드 수정 및 커밋메시지 수정 완료 후 해피 새로 머지해주신것까지 리베이스해서 다시 푸쉬 했습니다!

@hyeonheebee hyeonheebee force-pushed the feat/72-MainView_dailyTarget_textChange branch 2 times, most recently from bf47bcc to 76205e2 Compare November 20, 2024 01:48
feat: #72 일일 기록에 따른 텍스트 수정

feat: #72 일일 기록에 따른 텍스트 수정 푸쉬 전

feat: #72 일일 기록에 따른 텍스트 수정 코드 단순화_함수변경

feat: #72  불필요 코드 삭제

feat: #72  커밋메시지수정
@hyeonheebee hyeonheebee force-pushed the feat/72-MainView_dailyTarget_textChange branch from 76205e2 to e0442a3 Compare November 20, 2024 10:52

// 텍스트 변경을 위한 추가 코드
// todayRecords이 nil이 아니면 todayRecords가 할당
if let todayRecords = todayRecords {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if let todayRecords = todayRecords {
if let todayRecords {

옵셔널 벗길 때 변수명 같게 쓰는 경우는 그대로 써도 됩니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어색해보였는데 알려주셔서 너무 감사합니다! +_+ 요것도 반영하고 바로 푸쉬해서 머지할게요!

@hyeonheebee hyeonheebee merged commit 5098fba into develop Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

오늘 할당량 입력하면 위클리 뷰 텍스트 변경하기
2 participants