-
Notifications
You must be signed in to change notification settings - Fork 36
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
AI-638: FieldSet Component #2053
Conversation
Snapshots were updated. Please verify the changes match the expected layout.
|
…er/backpack-ios into chai/AI-638_field-set-component * 'chai/AI-638_field-set-component' of github.com:Skyscanner/backpack-ios: Updated snapshots
|
||
// swiftlint:disable closure_body_length | ||
@ViewBuilder | ||
private func constructViews(inErrorState: Bool) -> some View { |
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.
this code should not be here, it should be contained within the previows. here it's accessible from the component and that should not be the case.
you could create only one #preview and then assigne names to the 2 scroll views it'd have inside!
import SwiftUI | ||
import UIKit | ||
|
||
public protocol BPKFieldSetStatusHandling: View { |
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.
minor: could you rename this protocol to be BPKFieldSetContentView
?
var body: some View { | ||
ScrollView { | ||
VStack(spacing: .md) { | ||
Text("With Label & Description").fontWeight(.bold) |
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.
Could you change these to be BPKText?
} | ||
} | ||
|
||
struct FieldSetExampleView_Previews: PreviewProvider { |
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.
you could use the new #preview macro instead
VStack(alignment: .leading, spacing: .sm) { | ||
labelView | ||
content | ||
.inputState(state) |
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'm not quite sure I understand how this works? the implementations in the Text field, text area and BPKSelect just return mapping a state? but how is the style applied in the end I dont understand
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.
Just got it! it's calling each element's method!
public func inputState(_ state: BPKFieldSet<BPKTextArea>.State) -> BPKTextArea { | ||
switch state { | ||
case .default: | ||
return inputState(BPKTextArea.State.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.
is it calling itself? is this not an infinite loop?
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.
Test
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.
Ahh.. it's not calling itself, but the component's method, okay sorry!
* main: AI-638: FieldSet Component (#2053) Add decision doc for accessibility identifiers in UI-tests (#2050) DON-591: Contribute Search Control Input component (#2038) Bump @babel/preset-env from 7.25.3 to 7.25.4 (#2051) Bump rexml from 3.3.3 to 3.3.6 (#2052) # Conflicts: # Backpack-SwiftUI/FieldSet/Classes/BPKFieldSet.swift # Backpack-SwiftUI/FieldSet/Classes/BPKFieldSetState.swift # Backpack-SwiftUI/FieldSet/README.md # Example/Backpack/SwiftUI/Components/FieldSet/FieldSetExampleView.swift
Contribue BPKFieldSet component. Figma design.
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