zenangst / spots Goto Github PK

I find it a bit confusing to take a step backwards and introduce a new naming convention for classes. Having everything share a spot prefix or suffix is a bit cumbersome and confusing at times. Maybe it would be better to stick to a convention that is more familiar to the platform or just more generic instead of forcing a convention that couples directly to the pod name.

What do you guys think?

@RamonGilabert @vadymmarkov

There are some errors in Spotify example.

I am playing with this code in the playground. I want to have different kinds of tableview cells displayed. When I enter a value for kind, I get an error that the reuseidentifier is not registered. So here is the recourse I have taken: check the content type based on the meta information.

But this is obviously not the correct way of doing things. Ideally I should have different view models (with different meta info), but be able to generate different types of tableviewcells using the view model.kind.

I did go through your examples. But they appear to be designed to show the power of your framework. Can you provide a simple sample listing different kinds of cells?

Could you also document more clear-cut definitions of components, spots, ... It is a little hard to discern what the different entities are within the framework and what purpose they serve.

Thanks.

public class ListCell: UITableViewCell, SpotConfigurable {

    public var size = CGSize(width: 0, height: 60)
    public var item: ViewModel?

    lazy var selectedView = UIView().then {
        $0.backgroundColor = UIColor.blueColor().colorWithAlphaComponent(0.4)
    }

    public override init(style: UITableViewCellStyle, reuseIdentifier: String!) {
        super.init(style: .Subtitle, reuseIdentifier: reuseIdentifier)
        selectedBackgroundView = selectedView
    }

    public required init?(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    public func configure(inout item: ViewModel) {
        backgroundColor = UIColor.whiteColor()
        textLabel?.textColor = UIColor.blackColor()
        detailTextLabel?.textColor = UIColor.blackColor()

        if let action = item.action where action.isPresent {
            accessoryType = .DisclosureIndicator
        } else {
            accessoryType = .None
        }

        if let contact = item.meta["contact"] as? Contact {
            textLabel?.text = contact.name
            detailTextLabel?.text = contact.rawPhones.first
        } else {
            textLabel?.text = item.title
            detailTextLabel?.text = item.subtitle
        }

        item.size.height = item.size.height > 0.0 ? item.size.height : size.height
    }

    public override func layoutSubviews() {
        super.layoutSubviews()

        textLabel?.x = 16
        detailTextLabel?.x = 16
    }
}

The composition feature is not very well tested and hence it is not optimal. When we tested this on tvOS, we quickly saw that composition is heavy reliant on the size. This causes problems when serving up view models from the backend. I think this needs to be improved by handling it internally if the size property is not set. I think we could do the same thing as we do for Spotable objects. If usesDynamicHeight is set to false (which is default), then Spots would computed all the sizes for all the items that are of composite type.

I think a good start would be to add more tests for this feature to check that it works correctly and then add another example to demonstrate the feature.

Now that we have tests, we should invite Travis CI to this repo.
Probably gonna regret this issue but what the heck!

Got compile error when trying to run the SpotsDemo project.
"Command failed due to signal: Segmentation fault: 11"

Looks like Xcode 8.1 is unhappy with "Sugar" - particularly the Localization file.

dependencies-path /Users/navisingh/Library/Developer/Xcode/DerivedData/SpotsDemo-ccnfuqrrimqxvjecjmciykwssoyu/Build/Intermediates/Pods.build/Debug-iphonesimulator/Sugar.build/Objects-normal/x86_64/Localization.swiftdeps -o /Users/navisingh/Library/Developer/Xcode/DerivedData/SpotsDemo-ccnfuqrrimqxvjecjmciykwssoyu/Build/Intermediates/Pods.build/Debug-iphonesimulator/Sugar.build/Objects-normal/x86_64/Localization.o

0 swift 0x000000010903ca4d PrintStackTraceSignalHandler(void*) + 45
1 swift 0x000000010903c476 SignalHandler(int) + 470

There is a known issue in iOS that viewWillTransitionToSize is also called when the presented view controller is rotated, which causes unwanted size change handling in SpotsController

For now https://github.com/hyperoslo/Spots/blob/master/Sources/iOS/Classes/SpotsController.swift#L273, we 're checking for guard presentedViewController == nil else { return }. But there is an edge case when at the end of presented view controller dismissal, this checking is wrong

I think we need to rely on some other properties to check, instead of presentedViewController, or a custom flag. What do you think @hyperoslo/ios

I always found it weird and odd that render() is a function that returns the view for the Spotable object. I think it would be nicer and more natural if the Spotable object had a computed property called view.

This would very much be a breaking change but I well worth one in my opinion.
Maybe this could be something that we go for in 6.0.0.
We do have some other breaking changes that we would like to implement as well (see the issue about the protocols), and the naming convention of delegate methods.

Would it be neat if we added a script to generate a swift file with all the components that you have in your project so that you don't have to manually assign them in your SpotConfigurator for each project?

I see some drawbacks of it, that it will be tied to the class name in some way and be done via convention. So if the views name is ProfileGridItem, the identifier would look something like profile-grid-item. But maybe that is not a problem as they would always be unique.

What do you guys think? Do you see any disadvantages with that approach ?

In Spots we only use a spares amount of all the functionality in Sugar so I'm thinking about removing it as a dependency. I guess the main thing that we use in Spots is the Grand Central Dispatch sugar, but if you think about it. You probably don't need all the features there either. Seeing that this is a UI related framework, I think that you would only need an interactive background thread and the main thread. When would you perform a background operation with low priority when it is UI related, never right?

I think it would be handy to implement lifecycle methods on Component so that we could target and trigger operations based on that. We could also add delegate methods to give the developer more control over those operations. I think this could play a big role in how the CoreComponent objects are prepared and with some delegation you could introduce things like content operations more easily than you can do now.

func componentWillAppear(_ component: Component)
func componentDidAppear(_ component: Component)
func componentWillDisappear(_ component: Component)
func componentDidDisappear(_ component: Component)

This idea was sparked while me and @vadymmarkov were looking into React Native yesterday, in React, all component have their own lifecycle which can be incredibly powerful if done right.

What do you guys think? @hyperoslo/ios

Development has been a bit to wild-wild-west-ish for a while now, I think we need to add more tests to get better regression testing in the future. And to cover all the moving parts as well!

Can't install from cocoapods because the specs are not available in their library.

https://cocoapods.org/?q=spots
https://cocoapods.org/?q=imaginary

It's useful to play with the potential structure of a page, for instance in the init here: https://github.com/hyperoslo/Spots/blob/master/Examples/Apple%20News/AppleNews/Controllers/ExploreController.swift

After playing with the components, i'd like to see how it would appear if pulled from a server as a JSON file.

Is there a way to convert the programmatically created components into JSON?

One now needs to include:
Spots, Brick, Tailor, Cache, And CryptoSwift frameworks for carthage. Used to be just Spots, Brick, and Tailor.

I'm trying to reload the first cell in the first spot, but https://github.com/hyperoslo/Spots/blob/master/Sources/iOS/Library/Listable.swift#L188

the indexes parameter seems to be unused

Hi, Thank you for your great work :)

I run into a crashing issue ONLY on iPhone6/s Plus devices, the CarouselSpot or GridSpot crashes after scrolling, the crash point is in SpotsScrollView.swift -> layoutSubviews() function

Here is a screenshot :

I think it 's good that we can remove all references to UIScreen to get the size. Spots controller should depend on its parent controller to get the size. This also helps to support parent - child view controller relationship

I tried the following Cartfile:

github "hyperoslo/Spots" ~> 1.2.0
github "hyperoslo/Sugar" ~> 1.0.13
github "hyperoslo/Brick" ~> 0.4.0
github "zenangst/Tailor" ~> 0.10.0

But got the error:
"Failed to discover shared schemes in project Sugar.xcodeproj—either the project does not have any shared schemes, or xcodebuild never returned"

I can't use the pod because I can't use use_frameworks in my Podfile courtesy openSSL which is used in one of the pods in my pod file.

When we implemented willDisplay and endDisplay, we included the view into the delegate method.
I think there is potential to include the view that was tapped in didSelect(item: Item, in spot: Spotable) as well.

So it would end up looking something like this:

func spotsDidSelect(view: SpotView, item: Item, in spot: Spotable) {

This would kind of make the method into a trio instead of having one of them standout for being less "cool".

What do you think? @hyperoslo/ios

Updated

I updated the example to include the framework name.

So I had this idea, what if we got rid of core types, like CarouselSpot, GridSpot etc. And instead the layout would configure the Spot to use what ever UI is appropriate based on kind. This way it would be easier to test as configuration would be handled by something else than the Spotable object. I'm pretty sure that this would reduce the amount of code duplication that we have right now with all the core types.

What this would mean is that we get rid of core types and use one polymorphic object type (preferably called Spot) that would render itself appropriately based on layout instructions.

We've been using SpotsDispatchQueue for long time now https://github.com/hyperoslo/Spots/blob/master/Sources/Shared/Structs/GrandCentralDispatch.swift, but as Foundation in Swift 3 has a dedicated support for GCD with DispatchQueue, we can just use the native ones instead

pod --version => 0.39.0

1. in Spots/Examples/SpotsCards and use pod install
   Fetching podspec for Spots from ../../
   [!] Unable to find a specification for Sugar depended upon by Compass
2. and using pod 'Spots', git: 'https://github.com/hyperoslo/Spots.git'. also give error
   [!] Unable to find a specification for Sugar depended upon by Compass
3. also if creating my own dependencies it create error
   https://raw.githubusercontent.com/steve21124/Compass/master/Compass.podspec

Right now, we have hardcoded values for sizes, like

public class GridSpotCell: UICollectionViewCell, SpotConfigurable {

  /// The preferred view size for the view
  public var preferredViewSize = CGSize(width: 88, height: 88)
...
}

I think we can put them in a structs, not really for them being configurable, but make the size explicit. This also make the test less magic numbers, like expect(height).to.equal(count * Dimensions.gridCell.size.height)

Explicitly versioning the dependencies will avoid situations like we have currently where the pod is broken because one of the dependency (crypto swift) has moved on to swift 3.0.

Currently, different delegate methods are called in various places to notify the API users of scrolling events, depending on whether the scrolling was animated or not. Example in CarouselScrollDelegate:

func spotableCarouselDidEndScrolling(_ spot: Spotable, item: Item)
func spotableCarouselDidEndScrollingAnimated(_ spot: Spotable)

Having both of these methods both leads to inconsistency (the first one accepts an Item where the scond one doesn't), and makes the API slightly harder to implement (2 methods vs 1). The proposal is to unify both of these methods in all places where this pattern is used to instead become the following:

func spotableCarouselDidEndScrolling(_ spot: Spotable, item: Item, animated: Bool)

Which simply sends whether the event occured with an animation as a parameter.

I made separate issues for the other improvements to the API but it might be better to combine them with one for each protocol.

So for ScrollDelegate it would look something like this:

Before

func didReachBeginning(in scrollView: ScrollableView, completion: Completion)
func didReachEnd(in scrollView: ScrollableView, completion: Completion)

After

func spotsDidReachBeginning(in scrollView: ScrollableView, completion: Completion)
func spotsDidReachEnd(in scrollView: ScrollableView, completion: Completion)

The new proposal for willDisplay would look something like this;

func spotsWillDisplay(view: SpotView, item: Item, in spot: Spotable)

Hi,

I'm getting this crash running the SpotsNibDemo or just adding even an empty .xib to any ios project that use Spots.

2016-11-28 21:08:53.201 SpotsNibDemo[484:13917030] *** Assertion failure in -[UINib initWithNibName:directory:bundle:], /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKit_Sim/UIKit-3600.5.2/UINib.m:94
2016-11-28 21:08:53.204 SpotsNibDemo[484:13917030] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid parameter not satisfying: (name != nil) && ([name length] > 0)'

The problem seams to be the super init call in the Controller init

super.init(nibName: nil, bundle: nil)

but I don't know how to pass here the right nibName.

Can you please help me with that?

Thank you so much.
Jacopo

I set in Cartfile
github "hyperoslo/Spots"

Error:
*** Building scheme "Spots-iOS" in Spots.xcodeproj
** BUILD FAILED **
...Carthage/Checkouts/Spots/Spots.xcodeproj -scheme Spots-iOS -configuration Release -sdk iphoneos ONLY_ACTIVE_ARCH=NO BITCODE_GENERATION_MODE=bitcode CODE_SIGNING_REQUIRED=NO CODE_SIGN_IDENTITY= CARTHAGE=YES clean build) failed with exit code 65:
** BUILD FAILED **

I think the new protocol methods should look like this:

func spotsDidChange(spots: [Spotable])

Same principal as #391

tvOS is temporary not supported since 3.0.3, there are some errors that need to be fixed.

It would be super nifty if Spots featured an internal caching mechanism, both on an entire controller and on a single spot. This would be entirely optional, it should default not to use it was it might not be needed for all views. So you could have an optional init method that instantiates a component or controller with a cache id, if components and view models exist, it will use those so that it doesn't need to wait for the network to fetch, parse and process everything into view model data.

I think that this could be a great and nifty improvement, Cache would be an ideal component for this use case.

This is what the new proposal for endDisplay would look like:

func spotsEndDisplay(view: SpotView, item: Item, in spot: Spotable)

We have some "legacy" methods that are still a bit unclear, things like setup(_ size) and layout(_ size). As @onmyway133 pointed out in a PR, these methods deserve some improvements.

Here are some suggestions on how to improve them:

Example 1

fun setupWithSize(_ size: CGSize)

Example 2

fun setup(with size: CGSize)

Example 3

fun setup(withParentSize size: CGSize)

Reference

https://github.com/hyperoslo/Spots/pull/468#pullrequestreview-20878625

I think we can make Spots safer

• Use more optional, and remove the usage of forced unwrapping optional
• Check for accessing element in array

Like https://github.com/hyperoslo/Spots/blob/master/Sources/iOS/Library/Spotable.swift#L97

public func item(index: Int) -> ViewModel {
    return component.items[index]
}

We need to check for index before accessing. We can extension Array to do this

The downside is that we have lots of optionals to deal with 😅

To eliminate how dependent we are on meta when using Item. It might be a good idea to move it back to Spots and add more mappable structs. The same approach that we are doing for Component when it comes to Interaction and Layout. That make it a lot easier to test and I think we can reduce the dependency that we have on meta a lot. I think it should be used more for edge-cases instead of something that we always depend on.

Trying to see how to extend this spots for nstextfield. any idea?

This where I think it will be implemented, feel free to let me know if it is not the correct way
https://github.com/hyperoslo/Spots/blob/master/Sources/Shared/Library/TypeAlias.swift

#if os(OSX)
  public typealias View = NSView
  public typealias ScrollView = NoScrollView
  public typealias TableView = NSTableView
  public typealias CollectionView = NSCollectionView
  public typealias TextField = NSTextField
#else
  public typealias View = UIView
  public typealias ScrollView = UIScrollView
  public typealias TableView = UITableView
  public typealias CollectionView = UICollectionView
  public typealias TextField = UITextfield
#endif

Since I want it to be list of text fields. I will need to modify Listable to include textField
https://github.com/hyperoslo/Spots/blob/master/Sources/Shared/Library/Listable.swift

/// Gridable is protocol for Spots that are based on UICollectionView
public protocol Listable: Spotable {

   var tableView: TableView { get }
   var textField: TextField { get}
}

then... it become interesting
https://github.com/hyperoslo/Spots/blob/master/Sources/Mac/Spots/ListSpot.swift
ListSpot need to conform to protocol Listable. since textField added in Listable protocol. where will be the right place to add textField in ListSpot?

When @JohnSundell's new CarouselSpot implementation is merge, it pretty much renders the current implementation of Paginate useless as we now share the same behaviour for both .page and .item based scrolling. However, in that implementation we assume that the target view should always be centered on screen. We could consider keeping Paginate but change it's values to be:

enum Paginate: String {
  case left, center, right, disabled
}

Depending on what paginate option you decide to go with, the snap-to feature would then adjust the targetContentOffset.pointee based on the desired alignment.

I don't see this as something crucial to add now as .center covers what we need at the moment and for me personally, it would be my default behaviour.

Just thought I'd write it down before the idea ran away and evaporated 😎

Currently, when the Controller is reloaded with [Component], the interface loses its current selection. This should not happen, I would even go as far as to call it a 🐛.

It would be super groovy if you no longer had to pick a core UI element to inherit from building view.
What this would mean is, if you are developing for iOS, all your project views would inherit from UIView instead of UITableViewCell or UICollectionView.

We would have to refactor Registry to available globally instead of a property on the spotable object. But I think that would just help when setting up the configuration for Spots. This would also mean that the views would be globally available and indifferent to the underlaying UI element used for rendering. One big advantage is that one view could be used everywhere, so only one view per identifier instead of multiple views sharing the same identifier but in different domains.

For me, this feels like the next logical step forward when it comes to Spots, and I'm super eager to get cracking on this issue.

The library is not documented well enough, I've started documenting all the methods and properties in this PR #289 but I think we can do much better than that. This will be an ongoing task and it is important to remember to always add docs when adding new classes, structs, methods or properties etc.

I began to realize that Spots can be used to create all-in-one app. For example consider, one app which does everything from cabs, food ordering, news reading, movies ticket booking etc etc and these apps can be installed as plugins in main app which in turn can be deleted too if user wanted.

Yes, the idea is similar to WeChat. Now my question is do we have this thing in mind or are we moving in that direction? I think we have a solid foundation to start.

I haven't tested it with grids, but I think it's the same issue there too. I see here https://github.com/hyperoslo/Spots/blob/6.0.0/Sources/iOS/Classes/ListSpot.swift#L41 we're trying to cast a cell to SpotConfigurable, which is not true in the case of a custom view and ListWrapper. But I think there are more places, because configure is not called for example after spot update.

Hi, for now we have View adopting SpotConfigurable, which confuses, at least for me at first. Because SpotConfigurable involves something that can be configurable on the Spotable object, instead of the View

public protocol SpotConfigurable: ItemConfigurable {

  /// The perferred view size of the view.
  var preferredViewSize: CGSize { get }

  func prepareForReuse()
}

Hi,
For iOS and macOS projets it's will be great to make your component Carthage compatible

Take a look at these articles:

• Ruthlessly Simple Dependency Management with Carthage
• Carthage Tutorial: Getting Started
• Making Carthage-compatible Frameworks with Swift
• Swift Cross Platform Framework (section Configure targets for Carthage) 👍

On your README.md add on top and add section for the installation with Carthage.

Sometimes we need to detect scrolling in the SpotsController subclass to do something, is it good to expose scrollViewDidScroll via delegate?

I've seen some crashes when a ViewModel.kind cannot be resolved in the adapters. This causes Spots to crash because it tries to dequeue something that hasn't been registered.

We need to improve the adapters to always default to the Spotable default view if the kind is not present inside the ViewRegistry.