[PM-27246] Update Authenticator to use ActionCard #2123
Conversation
|
Claude finished @matt-livefront's task —— View job Code Review CompleteSummaryThe PR successfully consolidates Status of Previous Review ItemsPrevious Claude Finding 1 (❌): Missing Findings
The style gained significant new functionality (size support with padding/font variations) but lacks test coverage. While the secondary button test was added for Suggested approachCreate tests verifying:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let message: String? | ||
|
|
||
| /// State that describes the secondary button. | ||
| let secondaryButtonState: ButtonState? |
There was a problem hiding this comment.
💭 Consider adding test coverage for the secondary button
While the moved test file includes tests for the action and dismiss buttons, there's no test verifying the secondary button's tap interaction. Consider adding test coverage for this new functionality in ActionCard+ViewInspectorTests.swift.
Suggested test implementation
@MainActor
func test_secondaryButton_tap() async throws {
var secondaryButtonTapped = false
let subject = ActionCard(
title: "Title",
message: "Message",
secondaryButtonState: ActionCard.ButtonState(title: "Secondary") {
secondaryButtonTapped = true
}
)
let button = try subject.inspect().find(asyncButton: "Secondary")
try await button.tap()
XCTAssertTrue(secondaryButtonTapped)
}
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2123 +/- ##
==========================================
- Coverage 85.22% 83.80% -1.43%
==========================================
Files 1708 1969 +261
Lines 145420 160852 +15432
==========================================
+ Hits 123940 134803 +10863
- Misses 21480 26049 +4569 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KatherineInCode
left a comment
KatherineInCode
left a comment
There was a problem hiding this comment.
Looks good, just one small question.
| ) | ||
| ) { | ||
| Image(decorative: SharedAsset.Icons.shield24) | ||
| .foregroundColor(Asset.Colors.primaryBitwardenLight.swiftUIColor) |
There was a problem hiding this comment.
🤔 Do we want to take this opportunity to update the colors here, as well?
There was a problem hiding this comment.
Yeah, great idea.
| shouldFillWidth: Bool = true, | ||
| size: ButtonStyleSize, | ||
| ) -> BitwardenBorderlessButtonStyle { | ||
| BitwardenBorderlessButtonStyle(size: size) |
There was a problem hiding this comment.
❌ Finding 1: The shouldFillWidth parameter is not being passed to the initializer.
The function accepts shouldFillWidth but doesn't use it, causing the parameter to have no effect. The struct property shouldFillWidth will always use its default value of false.
Suggested fix
static func bitwardenBorderless(
shouldFillWidth: Bool = true,
size: ButtonStyleSize
) -> BitwardenBorderlessButtonStyle {
BitwardenBorderlessButtonStyle(shouldFillWidth: shouldFillWidth, size: size)
}
| } | ||
|
|
||
| /// If this button should fill to take up as much width as possible. | ||
| var shouldFillWidth = false |
There was a problem hiding this comment.
BitwardenBorderlessButtonStyle gained significant new functionality (size support with padding/font variations), but there are no tests verifying this behavior. Consider adding tests similar to those for other button styles in the codebase.
fedemkr
left a comment
fedemkr
left a comment
There was a problem hiding this comment.
🤔 As Claude is suggesting, should we add snapshot tests for the new behavior in ActionCard and the button style?
🎟️ Tracking
PM-27246
📔 Objective
Fixes a spacing issue for the "Learn More" button in the "Sync with the Bitwarden app" card. I ended up moving
ActionCardinto BitwardenKit and adopting that in Authenticator, which brings this component up to the latest designs.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes