Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

sujileelea: Model, View 수정 및 Asset 추가 #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sujileelea
Copy link
Member

@sujileelea sujileelea commented Mar 29, 2024

Model

  • 기존 코드 초기값 수정
  • Board 모델 생성 및 수정
  • Task 모델 삭제
  • SwiftData를 이용한 데이터 관리 클래스 TaskData 모델 생성
  • SwiftUI를 이용한 UI 업데이트용 구조체 TaskItem 모델 생성
  • 위 두 모델 분리 이유는 drag and drop을 위한 Transferable 프토토콜이 구조체에만 적용되기 때문

View

  • 코드 전반 trim (Stack 요소 배치, 불필요한 코드 삭제 등)
  • TaskCard UI 개선
  • BoardPage에 drag and drop 기능 추가

Comment on lines +54 to +57
if let draggingItem, status, draggingItem != taskItem {
/// Moving Color from source to destination
if let sourceIndex = taskItems.firstIndex(of: draggingItem),
let destinationIndex = taskItems.firstIndex(of: taskItem) {
Copy link

Choose a reason for hiding this comment

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

중첩으로 if let 을 사용하는 것보다 guard let을 사용하는 것이 어떨까요?

Copy link

Choose a reason for hiding this comment

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

Suggested change
if let draggingItem, status, draggingItem != taskItem {
/// Moving Color from source to destination
if let sourceIndex = taskItems.firstIndex(of: draggingItem),
let destinationIndex = taskItems.firstIndex(of: taskItem) {
guard let draggingItem, status, draggingItem != taskItem else { return }
guard let sourceIndex = taskItems.firstIndex(of: draggingItem) else { return }
guard let destinationIndex = taskItems.firstIndex(of: taskItem) else { return }

Copy link
Member Author

Choose a reason for hiding this comment

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

코드가 간결해지겠네요 적용하겠습니다!

// @Query private var tasks: [Task]
let today = Date().formatted(.iso8601.year().month().day())
var tasks: [Task] = [Task(name: "one", category: "One"), Task(name: "two", category: "Two")]
@State var taskItems: [TaskItem] = [TaskItem(id: "1", name: "one", category: "iOS", isPinned: true, isDone: false), TaskItem(id: "2", name: "two", category: "Year", isPinned: true, isDone: false), TaskItem(id: "3", name: "three", category: "iOS", isPinned: true, isDone: false)]
Copy link

Choose a reason for hiding this comment

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

테스트용 더미데이터 인가요? extension 으로 빼두는게 좋아보입니다.

Suggested change
@State var taskItems: [TaskItem] = [TaskItem(id: "1", name: "one", category: "iOS", isPinned: true, isDone: false), TaskItem(id: "2", name: "two", category: "Year", isPinned: true, isDone: false), TaskItem(id: "3", name: "three", category: "iOS", isPinned: true, isDone: false)]
@State var taskItems: [TaskItem] = TaskItem.fetchMockItems()

Copy link
Member Author

Choose a reason for hiding this comment

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

평상시에 항상 저런식으로 냅다 선언해두는데 앞으로는 깔끔하게 빼두어야겠네요.

List {
ForEach(tasks) { task in
TaskCard(task: task, isOnBoard: true)
ScrollView {
Copy link

Choose a reason for hiding this comment

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

38번째줄 indent가 잘못 적용된거 같아요. 전체적으로 들여쓰기 한번씩 단축키로 체크해주세요

Copy link
Member Author

Choose a reason for hiding this comment

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

이런 부끄러운 실수를...지적해주셔서 감사합니다.

Comment on lines +46 to +48
//#Preview {
// TaskCard(task: TaskItem(name: "preview", category: "Preview"), isOnBoard: false)
//}
Copy link

Choose a reason for hiding this comment

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

안쓸거라면 주석보단 그냥 제거하는 것이 좋습니다. 사용할거라면 주석보단 수정하는 것이 좋구요!

Copy link
Member Author

Choose a reason for hiding this comment

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

네 수정하는 방향으로 적용하겠습니다!

Comment on lines 27 to 29
var id: UUID = UUID()
var name: String
@Attribute(.unique) var category: String
var category: String
Copy link

Choose a reason for hiding this comment

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

데이터 라이프사이클동안 값이 바뀌는 것 같진 않은데 let으로 하지 않은 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

let id
var name
@Attribute(.unique) var category

로 바꿔야겠네요. 코드로는 아직 구현이 안되어있지만 name과 category 프로퍼티는 값 수정이 가능합니다!

Comment on lines +10 to +11
import UniformTypeIdentifiers
import SwiftUI
Copy link

Choose a reason for hiding this comment

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

import 구문에도 나열 규칙이 있다면 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

전혀 고려하지 못했던 부분이네요. 퍼스트파티부터 순차적으로 나열하는 방법이 생각납니다.
혹시 재성님은 어떤식으로 하는지 여쭤볼 수 있을까요?

Copy link

Choose a reason for hiding this comment

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

선호도에 따라 다른데

보통 formatting이나 linting 규칙에서는 두가지를 사용합니다

  • 길이순
  • 알파벳순

전 보통 길이순을 선호합니다. 알파벳순하면 새 모듈 임포트할때마다 머릿속으로 abc 세야해서...

init(name: String, category: String) {
self.name = name
self.category = category
return category == "board"
Copy link

Choose a reason for hiding this comment

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

Suggested change
return category == "board"
category == "board"

Copy link
Member Author

Choose a reason for hiding this comment

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

디테일한 부분까지...🥹 감사합니다.

Comment on lines 38 to +39
VStack {
List {
ForEach(tasks) { task in
TaskCard(task: task, isOnBoard: true)
ScrollView {
Copy link

Choose a reason for hiding this comment

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

VStack 안에 ScrollView 밖에 없어서 불필요한거 같은데 의도하신게 아래 코드일까요?

ScrollView {
    LazyVStack { // lazy 하게 데이터 가져와서 불필요한 메모리 사용을 줄임
        // ForEach ...
    }
}

이게 아니라면 List 쓰는게 나아보입니다. 요즘 List 퍼포먼스 많이 개선된걸로 알아서요.

List(taskItems) { ... }

// 또는
List {
    ForEach(taskItems) { ... }  // 👈 이유는 모르겠으나 애플 샘플코드에는 List 를 이런식으로 쓰더라구요?
}

Copy link
Member Author

Choose a reason for hiding this comment

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

기존에는 List였으나 draggable이 적용이 안돼서 ScrollView로 바꿨습니다.
ScrollView는 기본적으로 vertical한 성질이니
필요한 메모리만 로드해 뷰에서 볼 수 있게 LazyVStack을 사용하려던 의도가 맞습니다!

Copy link

Choose a reason for hiding this comment

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

오 List는 draggable 적용이 안되는군요! 하나 배워갑니다~

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants