-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DON-788: Update BPKSearchInputSummary #2066
Conversation
42b9b3b
to
b2cd22e
Compare
Backpack-SwiftUI/AppSearchModal/Classes/BPKAppSearchModal.swift
Outdated
Show resolved
Hide resolved
Backpack-SwiftUI/AppSearchModal/Classes/BPKAppSearchModal.swift
Outdated
Show resolved
Hide resolved
Snapshots were updated. Please verify the changes match the expected layout.
|
Snapshots were updated. Please verify the changes match the expected layout.
|
/// - inputPrefix: The prefix which would be displayed on the left of text input | ||
/// - text: The text to display in the text field. | ||
private let readOnly: Bool | ||
private let clearAction: ClearAction? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per figma, this should not be optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before it was optional, I need to double check it with Adam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can make it non-optional
Backpack-SwiftUI/SearchInputSummary/Classes/BPKSearchInputSummaryState.swift
Outdated
Show resolved
Hide resolved
Backpack-SwiftUI/SearchInputSummary/Classes/BPKSearchInputSummary.swift
Outdated
Show resolved
Hide resolved
if let clearAction { | ||
let clearIcon = BPKSearchInputSummary.Icon(icon: .closeCircle, color: .textSecondaryColor) | ||
Button(action: clearAction.action) { | ||
BPKIconView(clearIcon.icon) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just do the definition of this here, and remove the BPKSearchInputSummary.Icon
struct altogether, it makes no longer sense
BPKIconView(.closeCircle)
.foregourndColor(.textSecondaryColor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this struct for a prefix, but yes, here we can simplify it and not use this struct for a clear action
Backpack-SwiftUI/SearchInputSummary/Classes/BPKSearchInputSummary.swift
Outdated
Show resolved
Hide resolved
Backpack-SwiftUI/SearchInputSummary/Classes/BPKSearchInputSummary.swift
Outdated
Show resolved
Hide resolved
Backpack-SwiftUI/AppSearchModal/Classes/BPKAppSearchModalState.swift
Outdated
Show resolved
Hide resolved
Backpack-SwiftUI/AppSearchModal/Classes/BPKAppSearchModal.swift
Outdated
Show resolved
Hide resolved
3de38ae
to
9ec8e24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! thanks for addressing the comments!
9ec8e24
to
403b738
Compare
@@ -20,47 +20,10 @@ import SwiftUI | |||
|
|||
extension BPKSearchInputSummary { | |||
/// The state of the text field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The state of the text field. | |
/// The style of the text field. |
private let inputPrefix: InputPrefix | ||
private var state: State = .default | ||
private let inputPrefix: InputPrefix? | ||
private var state: Style = .default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private var state: Style = .default | |
private var style: Style = .default |
Not an issue, just to be consistent with new naming/approach.
0e75fc1
to
fba421d
Compare
Snapshots were updated. Please verify the changes match the expected layout.
|
5f8a48c
to
213c5be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🚀
Niko already mentioned a few comments that you addressed it.
Additionally, for the integration, the focus state and UI tests function as required. ✅
Remember to include the following changes:
README.md
Backpack.h
header fileIf you are curious about how we review, please read through the code review guidelines