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.

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

Actions #183

Merged
merged 3 commits into from
Jul 21, 2020
Merged

Actions #183

merged 3 commits into from
Jul 21, 2020

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Jul 11, 2020

Introduces the concept of a ListStateObserver and ListActions for observing changes to a list, and for performing actions (currently only supported scrolling), respectively.

See the ListStateViewController for an example of it in action.

@kyleve kyleve changed the title [DRAFT] Actions Actions Jul 12, 2020
@@ -22,6 +22,8 @@ extension ListView

func collectionView(_ collectionView: UICollectionView, shouldHighlightItemAt indexPath: IndexPath) -> Bool
{
guard view.behavior.selectionMode != .none else { return false }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should set up some UI Tests for all this at some point...

/// If the item is contained in the list, true is returned. If it is not, false is returned.
///
@discardableResult
public func scrollTo<Content:ItemContent>(item : Identifier<Content>, position : ItemScrollPosition, animated : Bool = false) -> Bool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dup of AnyIdentifier version


guard let coordinator = coordinator else {
// No transition coordinator is available – we should just deselect return in this case.
item.set(isSelected: false, performCallbacks: true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is duped a couple places; consider unifying somewhere...

/// (for example, scrolling to a given item in the list) when used when you otherwise do
/// not have a reference to the underlying list view (for example, when using `ListViewController` or `BlueprintLists`).
///
/// You also gain access to a `ListActions` instance when using `ListStateReader`, in each registered callback.
Copy link

Choose a reason for hiding this comment

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

I have a little question about how to extract ListActions from ListStateReader given your example

list.stateReader = ListStateReader { reader in
    reader.onDidScroll { info in
        // Perform an action based on scrolling.
    }
}

Is the list action inside info?

Copy link

Choose a reason for hiding this comment

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

Oh I see, VisibilityChanged, DidScroll and ContentChanged all have list action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can also provide your own in the view controller, like the list state view controller does: https://github.com/kyleve/Listable/pull/183/files#diff-b65540c265cdb28b73523231548845db

For a Blueprint list, it would be similar:

List { list in
   ...
   list.actions = self.actions
}

@@ -8,83 +8,205 @@
import Foundation


///
Copy link

Choose a reason for hiding this comment

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

👍 Thank you for these docs!


/// Example of how to use the `actions` within a reactive framwork like Combine or ReactiveSwift.

cancel = $index.sink { [weak self] value in
Copy link

@DJBen DJBen Jul 13, 2020

Choose a reason for hiding this comment

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

It looks like thru using reactive framework with the list actions, we can achieve auto scrolling on insert externally with ease. Does that mean the auto scroll API might have been redundant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quite possibly; yeah – the auto-scroll is nice when you don't have conditional / signal based work to do, but yeah, might be nice to simplify the framework by pushing that requirement out to consumers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Could probably even implement one in terms of the other, I bet)

Copy link

Choose a reason for hiding this comment

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

I am neutral on whether the auto scrolling API should stay. I think it is definitely convenient for some people. For me working in a workflow, I would lean towards having more explicit control on when the list should scroll.

@DJBen
Copy link

DJBen commented Jul 17, 2020

Hey Kyle, let me know what is left to be done in this change, and when is the expected date for it to be merged. Thanks!

@kyleve
Copy link
Collaborator Author

kyleve commented Jul 17, 2020

Not a ton of stuff; need to apply the feedback here and then get the tests written. Hopefully landing this week (but it is hack week for us), but more likely Monday.

@kyleve kyleve force-pushed the kve/list-state branch 4 times, most recently from 51d71be to ff7c3bf Compare July 17, 2020 01:54
* origin/main:
  Opt out of layout if not vertical
  Additional self review and updates
  Self code review and tests for added code
  Remove LayoutDirection propagation from most APIs, implement rest of paged layout.
  Support keyboardAdjustmentMode, which allows for optionally not adjusting the content inset when a keyboard is visible
@kyleve kyleve marked this pull request as ready for review July 21, 2020 01:29
@kyleve kyleve merged commit 50b0af2 into main Jul 21, 2020
@kyleve kyleve deleted the kve/list-state branch July 21, 2020 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants