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

Merge 0.9 Version Dev Branch to Master #109

Merged
merged 24 commits into from
Feb 16, 2018
Merged

Merge 0.9 Version Dev Branch to Master #109

merged 24 commits into from
Feb 16, 2018

Conversation

wkiefer
Copy link
Contributor

@wkiefer wkiefer commented Feb 15, 2018

Biggest change is the great flattening: 305d49f

Subsequent diffs tweaked the design, added helpers, fixed up examples, added examples to CI, and more.

danielrhammond and others added 23 commits June 10, 2017 13:29
Split out sectioning behavior from `ModelCollection` into `SectionedModelCollection` to simplify all the model collection code that only needs to support a single section.
• Basic setup with Pilot dependencies, config files, and on-disk organization.
• Also added bare bones root window controller & view controller to mac.
• Added proper sidebar selection style.
• Added basic navigation action and router handling.
• Added basic Example MVVM stack for the content list.
- Xcode project file merge forgot two files.
- Small test fix for de-sectioning support of `ModelCollection`.
- Update to Swift 4
- Fix build warnings.
- De-sectioning support.
- Fix up description code written after the flattening.
- Add all example apps to CI.
- Fix framewok search paths so they custom overrides on Travis.
  This avoids build warnings inside Xcode for non-existent directories.
- Provide an array of `ModelCollectionState` rather than `Model` items.
- Conform to `ModelCollection` officially.
- Added helpers, and adjusted the CV data source.
- `ComposedModelCollection` can now compose sub-collections into a
  single section or as individual sections.
- These methods are generally unused and pre-date some significant
changes to `ModelCollection`. This also makes the section-wrapping
work as expected.
@wkiefer wkiefer added this to the 0.9 milestone Feb 15, 2018
@wkiefer wkiefer self-assigned this Feb 15, 2018
Copy link
Contributor

@danielrhammond danielrhammond left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -16,22 +16,22 @@ public final class ScoredModelCollection: ModelCollection, ProxyingCollectionEve
self?.handleSourceEvent(event)
}

updateSections()
updateModels()
}

// MARK: Public
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally out of scope for this diff but in retrospect this name ScoredModelCollection seems like it doesn't really describe what this MC does, though this is also one of the changes that looks really good in the flattening (sectionLimit -> limit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed — ScoredModelCollection is up for rename in #54 👍


/// `NSCollectionViewFlowLayout` which presents items as a full-width list. This class handles auto-invalidating itself
/// when the bounds change in a way that requires a new layout pass.
open class CollectionViewListLayout: NSCollectionViewFlowLayout {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does open make sense in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, fixed.

}
}

// TODO:(wkiefer) Replace when Pilot has a block-based view binding provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

We do have StaticViewBindingProvider(type: ExampleView.self) which would work here fwiw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

}
}

// TODO:(wkiefer) Replace when Pilot has a block-based view binding provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


// MARK: Public

public static func make(with context: CatalogContext) -> TopicCollectionViewController {
Copy link
Contributor

Choose a reason for hiding this comment

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

random idea, but since we do a lot of things like this in our app it might be nice to provide an optional protocol CVCs can conform to with some static funcs which if conformed to provide a convenience init(context:) initializer

static func defaultModel() -> ModelCollection
static func defaultLayout() -> NSCollectionViewLayout
static func viewBinder() -> ViewBindingProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice idea! filed #110

observers.notify(.didChangeState(state))
}
public var state: ModelCollectionState {
// `sectionedState` is the source-of-truth for this class.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe log.warn here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sorry. you can definitely call this — the comment was meant to explain why this is a dynamic getter. I'll update it to clarify.

@wkiefer wkiefer merged commit 6805c60 into master Feb 16, 2018
@wkiefer wkiefer deleted the 0.9-dev branch February 16, 2018 20:11
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.

None yet

2 participants