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

[TNT-139] TControlButton, TPopUp 컴포넌트 코드 작성 #21

Merged
merged 6 commits into from
Jan 19, 2025

Conversation

FpRaArNkK
Copy link
Contributor

@FpRaArNkK FpRaArNkK commented Jan 15, 2025

📌 What is the PR?

  • 할당 컴포넌트 중 컨트롤 버튼 세트와 팝업 화면을 작성했습니다.

🪄 Changes

  • 할당 컴포넌트 중 컨트롤 버튼 세트와 팝업 화면을 작성했습니다.

🌐 Common Changes

- 토글 관련 새로운 아이콘 이미지를 추가했습니다.

🔥 PR Point

  1. TControlButton 컴포넌트를 작성했습니다. Notion 상세 개발 문서
    on / off 상태에 따라 UI 표시가 변경되며 외부에서 on/off와 tapAction을 정의합니다.
    tapAction은 trailing Closure를 통해 정의 가능합니다.
    Toggle은 TToggleStyle로 분리하여, 기존의 Toggle에 UI 스타일을 적용합니다.
    사용법은 아래와 같습니다.
// Radio Button Example
TControlButton(type: .radio, isSelected: $isRadioSelected) {
    print("Radio Button Tapped. Selected: \(isRadioSelected)")
}
            
 // Check Mark Example
TControlButton(type: .checkMark, isSelected: $isCheckMarkSelected) {
    print("Check Mark Tapped. Selected: \(isCheckMarkSelected)")
}

// Toggle Example
Toggle("Example", isOn: $isOn)
            .applyTToggleStyle()
...
  1. TPopUp 컴포넌트를 작성했습니다. Notion 상세 개발 문서
    - TPopUp이라는 네임스페이스를 공유하며, Alert, ButtonContent, Modifier를 작성했습니다.
    - TPopUp.Modifier를 통해 외부에서 뷰를 주입받아 표시하거나, 사전 정의된 앱 Alert뷰를 사용할 수 있습니다.
  • stealmh님의 TCAPopupAlert를 참고하여, TCA에서 추적할 수 있는 상태 기반 Alert 표시 구조로 리팩토링을 진행했습니다.
  • Alert에 표시되는 title, message, button들을 Equatable한 상태로 관리하고, 이를 화면에 옮겨 표시합니다.
  • Alert 는 .tPopUp(isPresented:, content:)를 통해 View에 바로 추가가 가능합니다.
  • Alert 외로, 커스텀한 개별 뷰도 View extension을 통해 .tPopUp(isPresented:, content:)을 통해 바로 사용할 수 있도록 작성했습니다.
    사용법은 아래와 같습니다.
struct ContentView: View {
    @State private var isPopupPresented: Bool = false
    @State private var isPopupPresented2: Bool = false

    var body: some View {
        SomeView()
        .tPopUp(isPresented: $isPopupPresented) {
            CustomView()
        }
        .tPopUp(isPresented: $isPopUpPresented) {
            TPopUpAlertView(alertState: TPopupAlertState(
                title: "Test PopUp",
                message: "This is a test message for the PopUp component.",
                buttons: [
                    TPopupAlertState.ButtonState(
                        title: "Cancel",
                        style: .secondary,
                        action: EquatableClosure { 
                           print("Cancel tapped") 
                           isPopUpPresented = false
                        }
                    ),
                    TPopupAlertState.ButtonState(
                        title: "Confirm",
                        style: .primary,
                        action: EquatableClosure {
                            print("Confirm tapped")
                            isPopUpPresented = false
                        }
                    )
                ]
            ))
        }
    }
}

📸 Screenshot

기능 스크린샷
GIF
GIF

🙆🏻 To Reviewers

  • 네임스페이스로 묶어본 것이 처음인데 맞는 방식인지 헷갈리는 것 같습니다..! 좋은 방향일까요?
  • 하나의 파일 안에 컴포넌트 관련된 코드를 모두 모아두다 보니 보기가 조금 힘든 것 같습니다. 파일로 분리하는게 좋은 방향일까요..?
  • 코드 리뷰 내용 반영했습니다 감사드립니다!

💭 Related Issues

@FpRaArNkK FpRaArNkK added the ✨Feat 새로운 기능 구현 (새로운 로직 추가, UI 구현 등) label Jan 15, 2025
@FpRaArNkK FpRaArNkK self-assigned this Jan 15, 2025
Comment on lines 37 to 38
isSelected.toggle()
tapAction()
Copy link
Member

Choose a reason for hiding this comment

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

보통 이런 select들이 api 쏘는값이 많을텐데
네트워크 실패하면 다시 값 변경하는 로직을 추가하실건가요?? 저는 이또한 파편이라 생각합니다...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 저기에 toggle빼는 걸 깜빡했습니다..! 죄송합니다..ㅠㅠ
해당 부분은 tapAction()만 수행하며, 외부에서 변경되는 isSelected 값을 반영하는 것이 맞습니다!

  • 해당 컴포넌트에서는 Reducer의 State만을 반영하여 UI에 표시하고,
  • tap하는 경우 action을 호출하여 Reducer의 State를 변경 -> UI 반영
    의 흐름으로 생각하며 작성했습니다

/// 선택 상태에 따른 이미지 반환
func image(isSelected: Bool) -> Image {
switch self {

Copy link
Member

Choose a reason for hiding this comment

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

불필요한 띄어쓰기!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아앗...!! 수정하겠습니다 감사합니다!

Comment on lines 50 to 56
enum Style {
case radio
case checkMark
case checkbox
case toggle
case star
case heart
Copy link
Member

Choose a reason for hiding this comment

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

저는 여기에 토글이 왜 껴있는지 잘 모르겠네요

Copy link
Contributor Author

@FpRaArNkK FpRaArNkK Jan 15, 2025

Choose a reason for hiding this comment

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

위에서 멘션주신 부분에 답변드린 것과 같은 맥락으로,
SwiftUI에서 기본적으로 제공하는 Toggle(isOn: Binding<Bool>)의 경우
Toggle 내부에서 바인딩으로 넘겨준 isOn의 값을 토글하는 문제가 발생했습니다.
말씀주셨던 API 호출 문제도 있고, Reducer의 로직 반영도 이루어지지 않는 UI처리라서 고민하다가 버튼으로 변경했습니다.
하지만 사용하는 입장에서 익숙한가..는 잘 모르겠습니다.. 분리해서 별도 구현하는 것이 좋을까요?

Copy link
Member

@stealmh stealmh Jan 15, 2025

Choose a reason for hiding this comment

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

저라면 분리할 것 같아요.
원래 바인딩해서 쓰게끔 하는 애를 액션을 만들어 쓴다 라는 것이 어색해서요!

한번 작성해주신 스타일 열거형이 꼭 액션이 필요한게 무엇이 있을까, 바인딩에 의존하고 있는 것이 무엇일까 잘 판단하고 분리할 지 정해주세요 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이마를 탁 때리게 되네요.. 더 고민하겠습니다!

@stealmh
Copy link
Member

stealmh commented Jan 15, 2025

결국에 네임스페이스를 사용함으로써 TPop.DoSomething의 형태로 사용하게 될텐데 엄청 이상적인 형태는 아닌 것 같아요.
저도 변경될 여지가 있는 값들(Constants, CGFloat, Size 형태라던가.. 민서님 작성하신것처럼 Style등등)에서만 많이 사용해봤습니다.

최근에 저도 팝업관련한 화면을 만든 적이 있는데 참고하셔도 좋을 것 같아 링크 남겨드릴게요. 지금 작성해주신 코드랑 거의 비슷해요
https://github.com/stealmh/TCAPopupAlert

하나의 파일 안에 컴포넌트 관련된 코드를 모두 모아두다 보니 보기가 조금 힘든 것 같습니다. 파일로 분리하는게 좋은 방향일까요..?

벌써 보기가 힘들다고 느껴진다면.. 일부를 분리해보는 것도 좋은 방법일 것 같아요

@FpRaArNkK
Copy link
Contributor Author

결국에 네임스페이스를 사용함으로써 TPop.DoSomething의 형태로 사용하게 될텐데 엄청 이상적인 형태는 아닌 것 같아요. 저도 변경될 여지가 있는 값들(Constants, CGFloat, Size 형태라던가.. 민서님 작성하신것처럼 Style등등)에서만 많이 사용해봤습니다.

최근에 저도 팝업관련한 화면을 만든 적이 있는데 참고하셔도 좋을 것 같아 링크 남겨드릴게요. 지금 작성해주신 코드랑 거의 비슷해요 https://github.com/stealmh/TCAPopupAlert

하나의 파일 안에 컴포넌트 관련된 코드를 모두 모아두다 보니 보기가 조금 힘든 것 같습니다. 파일로 분리하는게 좋은 방향일까요..?

벌써 보기가 힘들다고 느껴진다면.. 일부를 분리해보는 것도 좋은 방법일 것 같아요

감사합니다! 첨부해주신 자료 참고해서 더 열심히 고민해보겠습니다.
이번에도 깊은 피드백 감사드립니다 🔥🔥 정진할게요!

.background(defaultPopUpBackgroundColor)
.cornerRadius(defaultCornerRadius)
.shadow(radius: defaultShadowRadius)
.padding()
Copy link
Member

Choose a reason for hiding this comment

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

padding 한번만 확인해주세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 부분 관련해서 확인한 결과, 현 스프린트 시점까지는 내부에 포함되는 컨텐츠 관련해서 -
alert 제외 동적으로 표시되는 화면이 없습니다. -> 해당 동적 컨텐츠 팝업 뷰의 고정 너비 or 외부 패딩 관련 정책이 없습니다.

현재 alert의 경우 내부 패딩을 적용한 상태로 고정 너비값을 가지므로 이를 메인으로 가져가면서,
추후 동적 컨텐츠이 포함된 디자인 확정 시 해당 정책 더블 체크해서 패딩값 바꿔놓겠습니다!

@FpRaArNkK FpRaArNkK merged commit f5b885f into develop Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨Feat 새로운 기능 구현 (새로운 로직 추가, UI 구현 등)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TNT-139] 할당 컴포넌트 작성
3 participants