[PM-38150] feat: Add Driver's License view screen#2758
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the read-only Driver's License view screen wired into Code Review DetailsNo findings. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2758 +/- ##
==========================================
- Coverage 87.85% 87.85% -0.01%
==========================================
Files 1712 1715 +3
Lines 166702 167058 +356
==========================================
+ Hits 146463 146769 +306
- Misses 20239 20289 +50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds the read-only detail view for driver's license items, rendering the license details section: first/middle/last name, license number, dates, issuing country/state/authority, and license class. The license number is masked with a reveal toggle; the name and license-number fields support copy. Wires the view into the per-type dispatch in ViewItemDetailsView.
SaintPatrck
force-pushed
the
vault/pm-38150-ios-license-view
branch
from
f0b24fd to
1b02754
Compare
| @ObservedObject var store: Store<DriversLicenseItemState, ViewItemAction, Void> | ||
|
|
||
| var body: some View { | ||
| if !store.state.isLicenseDetailsSectionEmpty { |
There was a problem hiding this comment.
♻️ Not to change int his PR, but @KatherineInCode @matt-livefront do you remember why we took this approach of checking if the specific cipher type details are empty inside the cipher type item view instead of doing it in ViewItemDetailsView wrapping each specific cipher type item view with the empty check?
There was a problem hiding this comment.
I don't really mind it here, creating the view is cheap and it keeps the empty check encapsulated within this view. If you move the check into the parent view, that view would need to reach into the child state to do the empty check.
|
|
||
| // MARK: Private Views | ||
|
|
||
| @ViewBuilder private var firstNameItem: some View { |
There was a problem hiding this comment.
⛏️ Add docs as well as the other view builder properties.
| PasswordVisibilityButton( | ||
| accessibilityIdentifier: "ShowDriversLicenseNumberButton", | ||
| isPasswordVisible: isVisible, | ||
| ) { | ||
| store.send(.driversLicenseItemAction(.toggleLicenseNumberVisibilityChanged(!isVisible))) | ||
| } |
There was a problem hiding this comment.
🤔 I know we're doing this on this way on Card, but I feel Identity/Login handles this better by not having to pass isVisible value here. I mean IMO the view should only send the processor .toggleLicenseNumberVisibilityChanged without parameters and then the processor check the current state and invert the visibility of the field.
I feel like doing it this way removes some logic from the View and it goes to the processor where it should live.
@bitwarden/team-ios @matt-livefront what do you think? Also I think we should have a tech-debt to update the Card approach as well.
There was a problem hiding this comment.
From a unidirectional data flow perspective, it would be better to not have the view do the toggling and dictate the new value to the processor. We're not very consistent on this though, and SwiftUI sometimes fights us. If you use a binding, the binding sends the new value as part of the action, which ends up being easier to use than to ignore it. I like updating this and the card cases though, it would end up being simpler than manually toggling it.
4 participants
🎟️ Tracking
📔 Objective
Adds the read-only detail view for Driver's License items. Renders the License details section: first/middle/last name (with copy), license number (masked with a reveal toggle + copy), date of birth / issue date / expiration date (formatted long dates), issuing country / state / authority, and license class. Empty fields are hidden. Wires the view into the per-type dispatch in
ViewItemDetailsView.📸 Screenshots