PM-16900: Update form card style for vault #1291
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1291 +/- ##
==========================================
- Coverage 89.47% 89.46% -0.02%
==========================================
Files 742 745 +3
Lines 46651 46814 +163
==========================================
+ Hits 41740 41880 +140
- Misses 4911 4934 +23 ☔ View full report in Codecov by Sentry. |
|
Great job, no security vulnerabilities found in this Pull Request |
| @@ -85,6 +85,14 @@ extension StyleGuideFont { | |||
| /// The font for the subheadline style. | |||
| static let subheadline = StyleGuideFont.dmSans(lineHeight: 16, size: 12, textStyle: .subheadline) | |||
|
|
|||
| /// The font for the subheadline semibold style. | |||
| static let subheadlineSemibold = StyleGuideFont( | |||
There was a problem hiding this comment.
🤔 Should this be added to the preview below (and therefore the snapshot tests)?
There was a problem hiding this comment.
While it's not exactly the same, the subheadline semibold font itself is already captured here:
ios/BitwardenShared/UI/Platform/Application/Appearance/StyleGuideFont.swift
Lines 273 to 274 in a2096e8
We could consider also adding subheadlineSemibold, but it doesn't quite align as well - how would you differentiate .styleGuide(.subheadline, weight: .semibold) vs .styleGuide(.subheadlineSemibold)?
I would prefer to not have to specify bold, semibold, etc styles and instead rely on .fontWeight() to specify the weight. But .fontWeight() can only be applied to Text instances rather than a View instance (before iOS 16). So for most cases you can use .styleGuide(.subheadline, weight: .semibold). But if you want to apply the style to a group of views, or in a button style then this subheadlineSemibold style is needed. So hopefully these extra styles can go away at some point.
There was a problem hiding this comment.
Ah, okay, that makes sense.
|
|
||
| /// The style for a borderless button in this application. | ||
| /// | ||
| struct BitwardenBorderlessButtonStyle: ButtonStyle { |
There was a problem hiding this comment.
🤔 Just like we have a snapshot test that has all the different font styles, are we getting to the point where it might make sense to have a preview and attendant snapshot test to showcase all the different button styles?
There was a problem hiding this comment.
Ah, that's an interesting idea!
There was a problem hiding this comment.
I added a snapshot test here, let me know if that's what you had in mind!
There was a problem hiding this comment.
❓ This text says "select Add TOTP" but there is no "Add TOTP" button to tap; should this copy be adjusted?
There was a problem hiding this comment.
Good catch!
| /// The font for the subheadline semibold style. | ||
| static let subheadlineSemibold = StyleGuideFont( | ||
| font: FontFamily.DMSans.semiBold, | ||
| lineHeight: 16, | ||
| size: 12, | ||
| textStyle: .subheadline | ||
| ) | ||
|
|
There was a problem hiding this comment.
🤔 What do you think about having special functions that alter some properties from the base font style guide creating a new one? Like in this case instead of building/specifying the whole style guide again we could take subheadline and just apply the font change to be semi bold. I'd think something like:
/// The font for the subheadline semibold style.
static let subheadlineSemibold = subheadline.with(font: FontFamily.DMSans.semiBold)
// ---- And in extension StyleGuideFont ----
/// Returns a new `StyleGuideFont` with same properties but different font.
func with(font: SwiftUI.Font) -> StyleGuideFont {
StyleGuideFont(
font: font,
lineHeight: lineHeight,
size: size,
textStyle: textStyle
)
}Then if subheadline size changes at some UI update, the semibold style will be updated as well automatically.
We could apply the same approach to ...bold styles in here as well.
There was a problem hiding this comment.
Yeah, that's a good idea!
628C
| init( | ||
| title: String? = nil, | ||
| titleAccessibilityIdentifier: String? = nil, | ||
| verticalPadding: CGFloat = 8, |
There was a problem hiding this comment.
🤔 I see verticalPadding not being used in this view, should the property/parameter be removed or should it be used in the contentView?
There was a problem hiding this comment.
Looks like it's not used, I removed it.
| Text(Localizations.enterKeyManually) | ||
| .styleGuide(.title2, weight: .semibold) | ||
|
|
||
| Text(Localizations.onceTheKeyIsSuccessfullyEntered) |
There was a problem hiding this comment.
There was a problem hiding this comment.
🤔 Shouldn't the custom field "Text empty" occupy the whole width?
There was a problem hiding this comment.
Yep, good catch.
| @@ -680,7 +680,7 @@ | |||
| "EnterKeyManually" = "Enter key manually"; | |||
| "AddTotp" = "Add TOTP"; | |||
| "SetupTotp" = "Set up TOTP"; | |||
| "OnceTheKeyIsSuccessfullyEntered" = "Once the key is successfully entered, select Add TOTP to store the key safely"; | |||
| "OnceTheKeyIsSuccessfullyEntered" = "Once the key is successfully entered, select Save to store the key safely."; | |||
There was a problem hiding this comment.
🤔 Given that the Key is not changing, we need to manually let Crowdin know the content changed so translations can be updated. I'll raise this in the meeting to discuss an approach to this. But I guess we should tag it in the Jira ticket so then someone can manually specify this in Crowdin.
There was a problem hiding this comment.
Does Crowdin do that automatically? I feel like we've changed a few other strings lately and it ends up resetting all of the localized versions of that string. Unless someone is manually doing that.
There was a problem hiding this comment.
Based on the meeting we'll check this but I think this PR can be directly merged in the meantime; and also we could check in future Crowdin syncs if this string is updated as a proof of how it's working now.
🎟️ Tracking
PM-16900
📔 Objective
Updates the card style background within the vault views along with a few other tweaks to match the updated designs.
BitwardenBorderlessButtonStylefor styling buttons displayed in a footer of a card.FieldLabelIconButtonStylefor styling icon buttons displayed in a field label within a card.BitwardenFieldandBitwardenTextFieldto support generic content in their footers.BitwardenToggleto support generic content in the title.📸 Screenshots
Add login:
View login:
⏰ 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