BIT-678: create UI for identity #199
Conversation
…eate-ui-for-identity
…ed snapshot testing
| case mr = "Mr" | ||
|
|
||
| /// Mrs title for married or widowed women. | ||
| case mrs = "Mrs" | ||
|
|
||
| /// Ms title for women regardless of marital status. | ||
| case ms = "Ms" | ||
|
|
||
| /// Mx title for individuals who prefer not to specify their gender or identify as non-binary. | ||
| case mx = "Mx" | ||
|
|
||
| /// Dr title for individuals with a doctoral degree. | ||
| case dr = "Dr" |
There was a problem hiding this comment.
@fedemkr Could you review these values and compare them to what are we saving in the live app?
Thanks!
There was a problem hiding this comment.
For context, @fedemkr , I was wondering if this should just be a text field, as honorific abbreviations vary wildly by language.
A more complex approach would be localizing the enum.
There was a problem hiding this comment.
ℹ️ I think I found an issue with this on our apps, I'll let you know as soon as I have a proper answer for this. Just in case to the user this should be presented localized but I see you have localizedName below so I guess that's already sorted out.
There was a problem hiding this comment.
yeah, I also did quick test around live Xamarin app, looks like it saves localized value directly, if user change the locale, it will show the old localized value, and when we try to edit identity, it will fail to preselect the title as new locale does not match the old one.
There was a problem hiding this comment.
Yes, exactly. We're discussing it internally on how we are going to handle that. I'll let you know as soon as I have the path forward.
There was a problem hiding this comment.
thanks, I think it would be better to merge this PR and follow up with new ticket for that.
There was a problem hiding this comment.
@ezimet-livefront the value saved for the title will be the English version of it so I think those values should be correct 😄
|
No New Or Fixed Issues Found |
| case `default` | ||
| /// placeholder default value of the type like `default` or `--select--` | ||
| /// - Parameter string: custom localizable title value for this default case, defaults to `Default`. | ||
| case `default`(String? = nil) |
There was a problem hiding this comment.
🤔 I think instead of adding the default value as an associated value, you could add it to the Menuable protocol as a static var like defaultValueLocalizedName (and a default value in an extension). This would be more consistent with how the localized name is for the custom values and you'd be able to use the default allCases value for types with a custom default value name.
/// A protocol that defines an object that can be represented and selected in
/// a `BitwardenMenuField`.
protocol Menuable: Equatable, Hashable {
/// A localized name value. This value is displayed in the Menu when the user
/// is making a selection between multiple options.
var localizedName: String { get }
static var defaultValueLocalizedName: String { get }
}
extension Menuable {
static var defaultValueLocalizedName: String {
Localizations.default
}
}
There was a problem hiding this comment.
Worked like a charm! 💯
Thanks!
Bitwarden code coverageTotal coverage:
|
| File | Coverage |
|---|---|
| BitwardenShared/Core/Platform/Models/Domain/DefaultableType.swift | 100.00% |
| BitwardenShared/Core/Vault/Models/Enum/TitleType.swift | 42.86% |
| BitwardenShared/UI/Platform/Application/Views/BitwardenMenuField.swift | 45.65% |
| BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditIdentityItem/AddEditIdentityItemView.swift | 88.76% |
| BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditIdentityItem/IdentityItemState.swift | 40.91% |
| BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemProcessor.swift | 95.18% |
| BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemView.swift | 99.28% |
| BitwardenShared/UI/Vault/VaultItem/CipherItemState.swift | 98.04% |
| BitwardenShared/UI/Vault/VaultItem/ViewItem/ViewLoginItem/Extensions/CipherView+Update.swift | 98.51% |
Powered by Slather
Generated by 🚫 Danger
…warden/ios into BIT-678/create-ui-for-identity
4704f51 to
9b0101b
Compare
| case mr = "Mr" | ||
|
|
||
| /// Mrs title for married or widowed women. | ||
| case mrs = "Mrs" | ||
|
|
||
| /// Ms title for women regardless of marital status. | ||
| case ms = "Ms" | ||
|
|
||
| /// Mx title for individuals who prefer not to specify their gender or identify as non-binary. | ||
| case mx = "Mx" | ||
|
|
||
| /// Dr title for individuals with a doctoral degree. | ||
| case dr = "Dr" |
There was a problem hiding this comment.
ℹ️ I think I found an issue with this on our apps, I'll let you know as soon as I have a proper answer for this. Just in case to the user this should be presented localized but I see you have localizedName below so I guess that's already sorted out.
| BitwardenTextField( | ||
| title: Localizations.firstName, | ||
| text: store.binding( | ||
| get: \.firstName, | ||
| send: AddEditIdentityItemAction.firstNameChanged | ||
| ) | ||
| ) | ||
| .textFieldConfiguration(.username) | ||
|
|
||
| BitwardenTextField( | ||
| title: Localizations.middleName, | ||
| text: store.binding( | ||
| get: \.middleName, | ||
| send: AddEditIdentityItemAction.middleNameChanged | ||
| ) | ||
| ) | ||
| .textFieldConfiguration(.username) | ||
|
|
||
| BitwardenTextField( | ||
| title: Localizations.lastName, | ||
| text: store.binding( | ||
| get: \.lastName, | ||
| send: AddEditIdentityItemAction.lastNameChanged | ||
| ) | ||
| ) | ||
| .textFieldConfiguration(.username) |
There was a problem hiding this comment.
🤔 I see these are using configuration .username but that has textInputAutocapitalization: .never which I think shouldn't be used in these cases given that usually first/middle/lastnames starts with capital letter so it would be a nice UX to have it automatically.
| } | ||
|
|
||
| /// Tests the add state with identity item filled with large text. | ||
| func test_snapshot_add_identity_full_fieldsFilled_largeText() { |
There was a problem hiding this comment.
🤔 I see not all fields are very long to exceed the size of the field box, could a test be added with very long texts for each property?
🤔 I see that the fields that are exceeding the size of the field box are being trimmed and I think that is not very accessible for people who use large text sizes, how would they read the whole text field? I think we should allow multiline to avoid these issues but check with design first if you can just in case they think differently.
There was a problem hiding this comment.
Hi @fedemkr, I think there is an issue with snapshot testing configuration with extra large accessibility font,
it actually allows multi line when I test it on simulator.
| /// The first name for this item. | ||
| var lastName: String = "" |
There was a problem hiding this comment.
| /// The first name for this item. | |
| var lastName: String = "" | |
| /// The last name for this item. | |
| var lastName: String = "" |
| /// The identity field was changed. | ||
| case identityFieldChanged(AddEditIdentityItemAction) |
There was a problem hiding this comment.
⛏️ Could you alphabetize this relative to the other actions?
| case let .identityFieldChanged(action): | ||
| switch action { | ||
| case let .firstNameChanged(firstName): | ||
| state.identityState.firstName = firstName | ||
| case let .middleNameChanged(middleName): | ||
| state.identityState.middleName = middleName | ||
| case let .titleChanged(title): | ||
| state.identityState.title = title |
There was a problem hiding this comment.
⛏️ What do you think about breaking out this switch into another function since it's already pretty long?
| /// The state for a login type item. | ||
| var loginState: LoginItemState | ||
|
|
||
| /// The state for a identity type item. | ||
| var identityState: IdentityItemState |
There was a problem hiding this comment.
⛏️ Can you alphabetize this relative to the other properties?
| let type: CipherType = .login | ||
| var type: CipherType |
There was a problem hiding this comment.
❓ Does this need to be a var? I don't think we have a reason to change this after it's initialized for the view screen.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Katherine Bertelsen <kbertelsen@bitwarden.com>
🎟️ Tracking
BIT-678
🚧 Type of change
📔 Objective
📋 Code changes
AddEditIdentityItemViewfor Identity.defaultcase📸 Screenshots
Screen Shots
⏰ 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