Add FXIOS-8383 [V124] Add a Custom Autofill Accessory View Supporting Credit Cards and Addresses #18584
Conversation
firefox-ios/Client/Frontend/Autofill/AutofillAccessoryView.swift
Outdated
Show resolved
Hide resolved
jnrahme
changed the title
nbhasin2
requested review from
cyndichin,
dataports and
razvanlitianu
and removed request for
dataports
|
could you also provide screenshots to the PR so I can compare if I see the same? thank you :) |
| /// Tint color for the accessory image view. | ||
| var accessoryImageViewTintColor: UIColor? { | ||
| get { | ||
| return accessoryImageView.tintColor | ||
| } | ||
| set { | ||
| accessoryImageView.tintColor = newValue | ||
| } | ||
| } |
There was a problem hiding this comment.
what do you think about making this view ThemeApplicable and setting the tintColor in applyTheme so that we're following this consistent pattern throughout the app? same for backgroundColor
There was a problem hiding this comment.
I was having the parent View AccessoryViewProvider take care of theming, not sure what the best practices for this is
firefox-ios/Client/Frontend/Autofill/AutofillAccessoryView.swift
Outdated
Show resolved
Hide resolved
| /// # Methods | ||
| /// - `init(image:labelText:tappedAction:)`: Initializes the accessory view with an image, label, and optional tap action. | ||
| /// - `tappedAccessoryButton()`: Handles the tap action on the accessory view. | ||
| class AutofillAccessoryView: UIBarButtonItem { |
There was a problem hiding this comment.
@jnrahme This seems like an over-engineered approach to having the button item when we can use a small factory method in the same class.
Not sure how this helps?
Edit: Just saw how you are using this!
There was a problem hiding this comment.
nit
| class AutofillAccessoryView: UIBarButtonItem { | |
| class AutofillAccessoryViewButtonItem: UIBarButtonItem { |
| } | ||
|
|
||
| self.useAccessoryTextLabel = .build { label in | ||
| label.font = DefaultDynamicFontHelper.preferredFont(withTextStyle: .title3, size: UX.fontSize, weight: .medium) |
There was a problem hiding this comment.
I also see this styling in the Figma for "Use saved address": iOS/Regular/Body, where do I confirm that it's title3 and medium?
There was a problem hiding this comment.
| Before | After |
|---|---|
 |
 |
This will create a table with two columns, each containing an image. Adjust the image URLs and width as needed.
@cyndichin its funny how I was looking at the same thing but in dark mode ha
Btw we should increase the hit area @jnrahme if possible and also add tint, I believe the button should have a tint when pressed as its current implementation has none
And the alignment seems off 🤔
| toolbar.sizeToFit() | ||
| // MARK: - UI Elements | ||
| private let toolbar: UIToolbar = .build { | ||
| $0.sizeToFit() | ||
| } | ||
|
|
||
| private lazy var previousButton: UIBarButtonItem = { |
There was a problem hiding this comment.
Should we use .build for UIBarButtonItem or is it not available?
There was a problem hiding this comment.
Good point @jnrahme did the build not work?
Usually build is for standard inits
There was a problem hiding this comment.
Going to r+ it regardless but try it
| @@ -134,19 +118,21 @@ class AccessoryViewProvider: UIView, Themeable { | |||
| // Reset showing of credit card when dismissing the view | |||
| // This is required otherwise it will always show credit card view | |||
| // even if the input isn't of type credit card | |||
| showCreditCard = false | |||
| currentAccessoryView = nil | |||
There was a problem hiding this comment.
What if we use standard as default?
There was a problem hiding this comment.
Oh I see now, we keep the view around and not AccessoryType.
|
Main branch this branch |
Client.app: Coverage: 31.9
Generated by 🚫 Danger Swift against 4a777f9 |
📜 Tickets
Jira ticket
Github issue
💡 Description
Created a versatile and reusable custom accessory view adhering to Figma designs. The accessory view should allow customization with images, text, and actions, offering flexibility for various use cases.
📝 Checklist
You have to check all boxes before merging