Skip to content
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.

Sign up for GitHub

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

Add async-CustomInput support #182

Merged
merged 5 commits into from
Jun 18, 2019
Merged

Add async-CustomInput support #182

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb11eba
Add async-`CustomInput` support
inamiy Jun 4, 2019
ad713ac
Rename to `customInputImmediately` and simplified
inamiy Jun 5, 2019
bd6df2f
Rename `customInputImmediately(when:)` to `customInput(immediatelyWhe…
inamiy Jun 14, 2019
ef12cd7
Rename `customInput(immediatelyWhen:)` to `autodisplayingCustomInput`
inamiy Jun 17, 2019
f789bcc
Revert example
inamiy Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions Bento/Decorators/CustomInputComponent.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import UIKit

extension Renderable {
/// Sets up `customInput` to prepare for presention when user taps `self`.
public func customInput(
_ input: CustomInput,
contentStatus: FocusEligibility.ContentStatus = .empty,
Expand All @@ -9,37 +10,66 @@ extension Renderable {
return CustomInputComponent(
source: self,
customInput: input,
contentStatus: contentStatus,
highlightColor: highlightColor
focusEligibility: .eligible(contentStatus),
highlightColor: highlightColor,
focusesOnFirstDisplay: false
).asAnyRenderable()
}

/// Sets up `customInput` to prepare for presention when user taps `self`,
/// and also presents it immediately when `state` is `.some`.
///
/// - important: This method is useful when `customInput` needs to be displayed asynchronously after state change.
/// - note: Due to asynchronous presentation, `focusEligibility` is not supported.
public func customInput<State>(
immediatelyWhen state: State?,
inamiy marked this conversation as resolved.
Show resolved Hide resolved
input: (State) -> CustomInput,
highlightColor: UIColor? = UIColor(red: 239/255.0, green: 239/255.0, blue: 244/255.0, alpha: 1)
) -> AnyRenderable {
return CustomInputComponent(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to change CustomInputComponent to support this? Why can't we just return self when state is nil? At least I think this should be done the same way that we build boxes where sections and/or nodes are conditionally composed depending on a certain condition and here I think we could follow the same principle, if the state is nil it means there's no CustomInput so we don't need a CustomInputComponent but once that changes we can then build a CustomInputComponent to display that input.

I'm not sure if I'm missing some detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state = nil doesn't mean we don't use customInput at all.
It's rather the same behavior as we already have: "Show customInput when user tapped"

On the contrary, when state exists, customInput shows immediately when re-rendered.
It also holds the same property "show customInput when user tapped" as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should rename method e.g. func customInput(immediatelyWhen:...) so that "immediately" belongs to state existence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state = nil doesn't mean we don't use customInput at all.
It's rather the same behavior as we already have: "Show customInput when user tapped"

I'm a bit confused, in your example you tap Gender and there's no input being presented, you display an activity indicator as accessory and after 1 sec. your state changes and then it shows the option picker, why do we need to have a custom input before the change of state?

Copy link
Contributor Author

@inamiy inamiy Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to have a custom input before the change of state

That's good question!
IIRC I tried the same approach by removing customInput only when loading.
But this caused Bento diffing animation for the entire "gender cell" that I wanted to avoid.
So, keeping customInput throughout the handling is (probably) required to avoid such animations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes it does that because we are swapping from one component to another one if we do that 😕

I don't see any way to avoid that animation but maybe @andersio has an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be some diffing optimization, but I think it's still OK to always keep customInput during loading.
It might still be able to tap though (which will be a bug), I can add isEnabled property to prevent that, so that we don't need to worry about removal animation of customInput.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For sure diffing optimization will be more nice, but we won't probably invest too much here)

Copy link
Contributor

@andersio andersio Jun 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#146 is meant to tackle that. But I'd be surprised that there is any deleting animation. It should be a subtle cross-fade.

source: self,
customInput: state.map(input),
focusEligibility: .ineligible,
highlightColor: highlightColor,
focusesOnFirstDisplay: state != nil
).asAnyRenderable()
}
}

struct CustomInputComponent: Renderable, Focusable {
let customInput: CustomInput
let customInput: CustomInput?
let focusEligibility: FocusEligibility
let highlightColor: UIColor?
let focusesOnFirstDisplay: Bool

let base: AnyRenderable

init<Base: Renderable>(
source: Base,
customInput: CustomInput,
contentStatus: FocusEligibility.ContentStatus,
highlightColor: UIColor?
customInput: CustomInput?,
focusEligibility: FocusEligibility,
highlightColor: UIColor?,
focusesOnFirstDisplay: Bool
) {
self.customInput = customInput
self.highlightColor = highlightColor
self.base = AnyRenderable(source)
self.focusEligibility = .eligible(contentStatus)
self.focusEligibility = focusEligibility
self.focusesOnFirstDisplay = focusesOnFirstDisplay
}

func render(in view: ComponentView) {
view.inputNodes = customInput
view.isFocusEnabled = focusEligibility.isEligible(skipsPopulatedComponents: false)
view.highlightingGesture.didTap = .manual
view.highlightingGesture.highlightColor = highlightColor
view.highlightingGesture.stylingView = view.containedView

view.bind(base)

if focusesOnFirstDisplay && view.canBecomeFirstResponder && !view.isFirstResponder {
_ = view.becomeFirstResponder()
}
}

func willDisplay(_ view: CustomInputComponent.ComponentView) {
Expand All @@ -65,6 +95,8 @@ extension CustomInputComponent {
}
}

fileprivate(set) var isFocusEnabled: Bool = true

var customInputView: InputView?
var focusToolbar: FocusToolbar?
var component: AnyRenderable?
Expand Down Expand Up @@ -103,7 +135,7 @@ extension CustomInputComponent {
public override func becomeFirstResponder() -> Bool {
if let nodes = inputNodes {
customInputView = InputView()
focusToolbar = FocusToolbar(view: self)
focusToolbar = FocusToolbar(view: self, isFocusEnabled: isFocusEnabled)
customInputView!.update(nodes)
}

Expand Down
14 changes: 9 additions & 5 deletions Bento/Views/FocusToolbar.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ public final class FocusToolbar: UIToolbar {
private let doneButton: UIBarButtonItem
private var view: (UIView & FocusableView)?

public init(view: UIView & FocusableView) {
public init(view: UIView & FocusableView, isFocusEnabled: Bool = true) {
backwardButton = UIBarButtonItem(
image: UIImage(named: "arrow_down", in: Resources.bundle, compatibleWith: nil)!,
style: .plain,
Expand All @@ -31,12 +31,16 @@ public final class FocusToolbar: UIToolbar {
forwardButton.action = #selector(forwardButtonPressed)
doneButton.target = self
doneButton.action = #selector(doneButtonPressed)
let items = [
forwardButton,
backwardButton,

var items = [UIBarButtonItem]()
if isFocusEnabled {
items.append(contentsOf: [forwardButton, backwardButton])
}
items.append(contentsOf: [
UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil),
doneButton
]
])

setItems(items, animated: false)
updateFocusEligibility(with: view)
}
Expand Down
23 changes: 23 additions & 0 deletions Example/SignUp/SignUpPresetner.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ final class SignUpPresenter {
navigator?.showAlert(title: "Did sign up", message: "\(state)")
}

/// - note: Simulates asynchronous picker presentation
inamiy marked this conversation as resolved.
Show resolved Hide resolved
func didTapGender() {
state.pickerState = .loading

DispatchQueue.main.asyncAfter(deadline: .now() + 1, execute: {
let serverResponse = Gender.allGenders // NOTE: assuming this is server response
self.state.pickerState = .showingPicker(serverResponse)
})
}

func didChangeGender(to gender: String) {
state.gender = gender
}
Expand Down Expand Up @@ -101,6 +111,7 @@ final class SignUpPresenter {
}
}

var pickerState: GenderPickerState = .idle

var isSignUpButtonEnabled: Bool {
return allFieldsAreFilledIn()
Expand Down Expand Up @@ -141,3 +152,15 @@ final class SignUpPresenter {
}
}
}

/// Async gender picker state.
inamiy marked this conversation as resolved.
Show resolved Hide resolved
enum GenderPickerState {
case idle
case loading
case showingPicker([Gender])

var showingPicker: [Gender]? {
guard case let .showingPicker(value) = self else { return nil }
return value
}
}
38 changes: 29 additions & 9 deletions Example/SignUp/SignUpRenderer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ final class SignUpRenderer {
|-+ section(id: .credential, text: "Credential")
|---+ email()
|---* passwordComponents(state)
|---+ gender(state.gender)
|---+ Node(id: .space, component: EmptySpaceComponent(spec: EmptySpaceComponent.Spec(height: 300, color: .clear)))
|---+ Node(id: .space, component: IconTextComponent(title: "(Scroll down)"))
|---+ Node(id: .space, component: EmptySpaceComponent(spec: EmptySpaceComponent.Spec(height: 300, color: .clear)))
|---+ gender(state)
|-? .iff(state.isSecurityQuestionsSectionVisible) {
self.section(id: .securityQuestion, text: "Security question")
|---+ self.securityQuestion(state)
Expand Down Expand Up @@ -113,19 +116,35 @@ final class SignUpRenderer {
]
}

private func gender(_ gender: String?) -> Node<RowID> {
private func gender(_ state: SignUpPresenter.State) -> Node<RowID> {
return Node(id: .gender, component:
Component.DetailedDescription(
texts: [.plain("Gender")],
detail: .plain(gender ?? "Choose"),
accessory: .none,
detail: .plain(state.gender ?? "Choose"),
accessory: {
switch state.pickerState {
case .idle:
return .chevron
case .loading:
return .activityIndicator
case .showingPicker:
return .none
}
}(),
didTap: { [presenter] in
presenter.didTapGender()
},
interactionBehavior: [],
styleSheet: descriptionStyleSheet
).customInput(
Component.OptionPicker(
options: Gender.allGenders,
selected: gender.map(Gender.init(displayName:)),
didPickItem: { self.presenter.didChangeGender(to: $0.displayName) }
)
immediatelyWhen: state.pickerState.showingPicker,
input: { genders in
Component.OptionPicker(
options: genders,
selected: state.gender.map(Gender.init(displayName:)),
didPickItem: { self.presenter.didChangeGender(to: $0.displayName) }
)
}
)
)
}
Expand Down Expand Up @@ -208,6 +227,7 @@ final class SignUpRenderer {
case securityQuestion
case info
case signUpAction
case space
}

enum RowID {
Expand Down