DON-384: Adjust place selector text field #1923
Conversation
…shots in light mode]
yurareutskiy
force-pushed
the
donburi/DON-384_adjust_place_selector_text_field
branch
from
3af3065
to
6de7a46
Compare
| @@ -54,14 +66,16 @@ class BPKAppSearchModalTests: XCTestCase { | |||
| // MARK: - Helpers | |||
| private func givenSut( | |||
| with results: BPKAppSearchModalResults, | |||
| inputState: BPKAppSearchModal.TextFieldState = .default | |||
| inputState: BPKAppSearchModal.TextFieldState = .default, | |||
| prefixState: BPKTextField.PrefixState? = nil | |||
There was a problem hiding this comment.
@yurareutskiy the test fails to compile on this line. The naming has probably changed.
This is also the reason the analyze step fails.
Once the tests run again, push an empty commit with Record snapshots to have CI take the snapshots for you. (If the snapshot test fails :D)
There was a problem hiding this comment.
Thanks! Still not sure why it was failing before I renamed that, but amazing as it's working:)
Snapshots were updated. Please verify the changes match the expected layout.
|
gert-janvercauteren
added
minor
…dd screenshots, fix a layout based on feedback]
| public struct BPKSearchInputSummary: View { | ||
| public enum InputPrefix { | ||
| case text(String) | ||
| case icon |
There was a problem hiding this comment.
this should receive a BPKIcon as parameter, so that clients can define what icon they use (clients of this component may be within backpack, but at least it would be configurable from outside this very component)
There was a problem hiding this comment.
hmm, that's a good idea I guess to make it more flexible
| .onAppear { | ||
| inputFieldIsFocussed.toggle() | ||
| } |
There was a problem hiding this comment.
I'm not sure we want this within the component, for example, hotels does not want this. it should be removed from here and leave it up to the clients defining this
There was a problem hiding this comment.
(if this is something you want to have)
There was a problem hiding this comment.
yeah, 100% should be removed 😅
| } | ||
|
|
||
| public var body: some View { | ||
| HStack { |
There was a problem hiding this comment.
Please specify spacing here using bpk tokens, like so:
| HStack { | |
| HStack(spacing: .md) { |
| BPKText(prefixText, style: .bodyDefault) | ||
| .foregroundColor(.textSecondaryColor) | ||
| case .icon: | ||
| BPKIconView(BPKIcon(name: "search")) |
There was a problem hiding this comment.
the icon should be passed by the ones using this component, shouldnt be hardcoded within. also, you should use the constant for the icon rather than a string for it
…ty to customise an icon for the prefix; remove testing code;]
Snapshots were updated. Please verify the changes match the expected layout.
|
| } else { | ||
| BPKIconView(icon.icon) | ||
| .foregroundColor(icon.color) | ||
| .accessibilityHidden(true) |
| .focused($focused) | ||
| accessory | ||
| } | ||
| .padding(.md) |
There was a problem hiding this comment.
Based on the design I think the padding is:
.padding(.left, .base)
.padding(.right, .md)
.padding(.vertical, .md)
There was a problem hiding this comment.
I thin it's actually .padding(.horizontal, .base).padding(.vertical, BPKSpacing.md.value + BPKSpacing.sm.value) (we don't have a token for value 12 hehe) could you raise the pr so I can approve it myself?
There was a problem hiding this comment.
The padding is 16 (.base) from left [leading], and 12 (.md + .sm) for all other sides.
Please correct me if I read the Figma file incorrectly. we will make the PR for this change after that, no worries
Figma link
inputPrefixwhich will allow us to set an additional visual enhancement for different verticals and their place selectors.inputPrefixcan be in two state: icon, customTextRemember to include the following changes:
README.mdBackpack.hheader fileIf you are curious about how we review, please read through the code review guidelines