[Feat] #135 - Dialog 컴포넌트 및 카탈로그 구현#137
Open
juri123123 wants to merge 5 commits into
Open
Conversation
1 task
WalkthroughMDSDialog와 Variant 타입이 추가되고, MDSCheckbox와 MDSFont의 공개 API가 확장됨. Storybook에 Dialog 카테고리와 데모 화면이 연결되어 여러 변형의 다이얼로그를 보여주도록 구성됨. ChangesDialog 컴포넌트 구현
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MDS/Sources/Components/Control/Checkbox/MDSCheckbox.swift (1)
66-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
title저장 프로퍼티가 init 시점에 동기화되지 않음.
init(size:title:)에서setTitle(title: title)을 직접 호출하지만self.title에는 대입하지 않습니다. 따라서MDSCheckbox(size:, title: "Label")로 생성한 인스턴스는 라벨은 정상 표시되지만, 이후.titlegetter를 읽으면nil이 반환되어 실제 UI 상태와 프로퍼티 값이 불일치합니다.🐛 제안하는 수정
public init(size: Size = .large, title: String? = nil) { self.checkboxSize = size super.init(frame: .zero) setup() - setTitle(title: title) + self.title = title }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MDS/Sources/Components/Control/Checkbox/MDSCheckbox.swift` around lines 66 - 73, `MDSCheckbox.init(size:title:)` is updating the UI via `setTitle(title:)` but leaving the stored `title` property unset, so the getter can return nil after initialization. Update the initializer to synchronize the backing `title` state before or alongside the existing `setup()` and `setTitle(title:)` calls, using the `MDSCheckbox` initializer and `title` property as the primary touchpoints.
🧹 Nitpick comments (2)
MDSStoryBook/MDSStoryBook/Component/Dialog/DialogViewController.swift (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
dialogs배열의 목적이 불명확합니다.
dialogs에 append만 되고 다른 곳에서 사용되지 않습니다. 뷰는 이미 뷰 계층(contentStack)에 강한 참조로 유지되므로 이 배열은 불필요해 보입니다.Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MDSStoryBook/MDSStoryBook/Component/Dialog/DialogViewController.swift` at line 13, The private dialogs array in DialogViewController is only appended to and never read, so it appears to be unnecessary state. Remove the dialogs storage and update the dialog creation path in addDialog(_: ) and any related cleanup logic to rely on the contentStack view hierarchy for ownership instead of keeping a separate array.MDS/Sources/Components/Dialog/MDSDialog.swift (1)
12-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
title/subTitle가 초기화 이후 갱신되지 않습니다.
public var title,public var subTitle는setUI()에서 최초 1회만titleLabel.text/descriptionLabel.text에 반영됩니다. 이후 외부에서 값을 재할당해도didSet이 없어 UI가 갱신되지 않습니다. 같은 스택의MDSCheckbox.title이didSet으로setTitle을 호출하는 패턴과 일관성이 없습니다.♻️ 제안하는 수정안
- public var title: String - public var subTitle: String + public var title: String { + didSet { titleLabel.text = title } + } + public var subTitle: String { + didSet { descriptionLabel.text = subTitle } + }Also applies to: 109-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MDS/Sources/Components/Dialog/MDSDialog.swift` around lines 12 - 13, `MDSDialog`의 `title`과 `subTitle`이 초기 설정 후 변경되어도 UI에 반영되지 않는 문제입니다. `MDSCheckbox.title`처럼 `MDSDialog`의 `title`/`subTitle`에 `didSet`을 추가해 값이 바뀔 때마다 `titleLabel.text`와 `descriptionLabel.text`가 즉시 갱신되도록 수정하고, 초기 `setUI()`에서도 동일한 반영 로직을 재사용하도록 정리하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MDS/Sources/Components/Dialog/MDSDialog.swift`:
- Around line 72-86: disableButton and dangerButton are initialized with
non-lazy let closures that reference self, so their addTarget targets may be
bound incorrectly. Update these properties in MDSDialog to lazy var, matching
primaryButton, and keep the same selector wiring so the targets are resolved
against the instance after initialization.
---
Outside diff comments:
In `@MDS/Sources/Components/Control/Checkbox/MDSCheckbox.swift`:
- Around line 66-73: `MDSCheckbox.init(size:title:)` is updating the UI via
`setTitle(title:)` but leaving the stored `title` property unset, so the getter
can return nil after initialization. Update the initializer to synchronize the
backing `title` state before or alongside the existing `setup()` and
`setTitle(title:)` calls, using the `MDSCheckbox` initializer and `title`
property as the primary touchpoints.
---
Nitpick comments:
In `@MDS/Sources/Components/Dialog/MDSDialog.swift`:
- Around line 12-13: `MDSDialog`의 `title`과 `subTitle`이 초기 설정 후 변경되어도 UI에 반영되지 않는
문제입니다. `MDSCheckbox.title`처럼 `MDSDialog`의 `title`/`subTitle`에 `didSet`을 추가해 값이
바뀔 때마다 `titleLabel.text`와 `descriptionLabel.text`가 즉시 갱신되도록 수정하고, 초기
`setUI()`에서도 동일한 반영 로직을 재사용하도록 정리하세요.
In `@MDSStoryBook/MDSStoryBook/Component/Dialog/DialogViewController.swift`:
- Line 13: The private dialogs array in DialogViewController is only appended to
and never read, so it appears to be unnecessary state. Remove the dialogs
storage and update the dialog creation path in addDialog(_: ) and any related
cleanup logic to rely on the contentStack view hierarchy for ownership instead
of keeping a separate array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3577643-1d4a-469a-9710-fcece93979b0
📒 Files selected for processing (6)
MDS/Sources/Components/Control/Checkbox/MDSCheckbox.swiftMDS/Sources/Components/Dialog/MDSDialog.swiftMDS/Sources/Components/Dialog/MDSDialogType.swiftMDS/Sources/Foundation/MDSFont.swiftMDSStoryBook/MDSStoryBook/Component/ComponentCategoryViewController.swiftMDSStoryBook/MDSStoryBook/Component/Dialog/DialogViewController.swift
yungu0010
requested changes
Jul 7, 2026
yungu0010
requested changes
Jul 7, 2026
yungu0010
left a comment
yungu0010
left a comment
Member
There was a problem hiding this comment.
컴포넌트 내 옵셔널 여부는 오른쪽 사이드바 참고하시거나 컴포넌트 클릭 후 옵셔널 버튼 -> 외부 영역으로 드래그 하면 확인하실 수 있습니다~!
| return button | ||
| }() | ||
|
|
||
| public init( |
Member
There was a problem hiding this comment.
저희 버튼 이미지(prefix icon, suffix icon)도 옵셔널이어서 생성자 주입으로 4개 더 추가해주세요!
added 2 commits
July 7, 2026 16:42
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MDS/Sources/Components/Dialog/MDSDialog.swift (1)
138-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win체크박스 간격이 variant와 무관하게 항상 24px로 고정되어 있습니다.
이전 리뷰에서 Danger variant의 체크박스 간격은 8px이어야 한다는 디자인 확인이 있었지만, 현재 코드는
checkBox가 있으면 variant에 상관없이setCustomSpacing(24, after: descriptionLabel)을 적용합니다.- if let checkBox { - stackView.setCustomSpacing(24, after: descriptionLabel) - stackView.addArrangedSubview(checkBox) - } + if let checkBox { + let spacing: CGFloat = variant.isDanger ? 8 : 24 + stackView.setCustomSpacing(spacing, after: descriptionLabel) + stackView.addArrangedSubview(checkBox) + }Based on past review discussion, "Danger에는 8px로 되어있어서... 8px로 맞춰달라고 하십니당" — variant별 분기가 필요한지 확인 부탁드립니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MDS/Sources/Components/Dialog/MDSDialog.swift` around lines 138 - 141, The checkbox spacing in MDSDialog is hardcoded to 24px regardless of variant, so update the checkBox handling to apply variant-specific spacing instead. Use the existing variant state in MDSDialog to branch the stackView.setCustomSpacing call so Danger uses 8px while the other variants keep the intended spacing, and keep the change localized to the checkBox block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MDS/Sources/Components/Dialog/MDSDialog.swift`:
- Around line 12-14: `MDSDialog` still treats the dialog description as
required, but it should be optional again. Update the `MDSDialog` stored
property currently named `subTitle` and the corresponding `init` parameter
`description` to be optional, then propagate that optionality through any usage
in the dialog content setup so dialogs can be created without description text.
Also check the related code paths around the referenced uses in the diff to
ensure they compile with the optional type.
---
Duplicate comments:
In `@MDS/Sources/Components/Dialog/MDSDialog.swift`:
- Around line 138-141: The checkbox spacing in MDSDialog is hardcoded to 24px
regardless of variant, so update the checkBox handling to apply variant-specific
spacing instead. Use the existing variant state in MDSDialog to branch the
stackView.setCustomSpacing call so Danger uses 8px while the other variants keep
the intended spacing, and keep the change localized to the checkBox block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbe11b1e-f255-4d99-bf25-600c07966b68
📒 Files selected for processing (4)
MDS/Sources/Components/Button/ActionButton/MDSActionButton.swiftMDS/Sources/Components/Dialog/MDSDialog.swiftMDS/Sources/Foundation/MDSFont.swiftMDSStoryBook/MDSStoryBook/Component/Dialog/DialogViewController.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- MDS/Sources/Foundation/MDSFont.swift
- MDSStoryBook/MDSStoryBook/Component/Dialog/DialogViewController.swift
Comment on lines
+12
to
+14
| private let title: String | ||
| private let subTitle: String | ||
| private let checkBox: MDSCheckbox? |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
description가 다시 필수(non-optional)로 되어 있습니다.
이전 리뷰에서 디자이너 확인 결과 description은 옵셔널이 맞다고 결론났고 작성자도 수정하겠다고 답변했으나, 현재 init의 description: String과 private let subTitle: String이 모두 non-optional로 남아있습니다. 설명 텍스트가 없는 다이얼로그를 구성할 수 없는 상태입니다.
🔧 옵셔널로 되돌리는 예시
- private let subTitle: String
+ private let subTitle: String?
...
public init(
variant: Variant,
title: String,
- description: String,
+ description: String? = nil,
checkbox: MDSCheckbox? = nil
) {
...
- descriptionLabel.text = subTitle
+ descriptionLabel.text = subTitle
+ descriptionLabel.isHidden = subTitle == nilBased on past review discussion, "문의 결과 옵셔널이 맞다고 합니다" — 확인 부탁드립니다.
Also applies to: 85-97, 109-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MDS/Sources/Components/Dialog/MDSDialog.swift` around lines 12 - 14,
`MDSDialog` still treats the dialog description as required, but it should be
optional again. Update the `MDSDialog` stored property currently named
`subTitle` and the corresponding `init` parameter `description` to be optional,
then propagate that optionality through any usage in the dialog content setup so
dialogs can be created without description text. Also check the related code
paths around the referenced uses in the diff to ensure they compile with the
optional type.
1 task
yungu0010
reviewed
Jul 8, 2026
yungu0010
left a comment
yungu0010
left a comment
Member
There was a problem hiding this comment.
수정사항 확인했습니다👍
코드래빗이 남긴 description 옵셔널 부분이랑 버튼 이미지(prefix, suffix 2종류) 삽입, 카탈로그 토글로 변경(또는 description 없는 버전도 추가) 작업이 남은 것 같아요!
세 개 작업 후 머지 부탁드립니다~~!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌴 PR 요약
🌱 작업한 브랜치
🌱 PR Point
체크박스에 사소한 오류가 있어 함께 수정했습니다.
attributedStringAttributes에 사소한 오류가 있어 함께 수정했습니다.
📌 참고 사항
기분탓인지.. 왜이리 폰트가 피그마와 달리 보일까요
📸 스크린샷
📮 관련 이슈