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

Add support for NSCollectionView & NSTableView #29

Merged
merged 7 commits into from
Sep 19, 2018

Conversation

OskarGroth
Copy link
Contributor

@OskarGroth OskarGroth commented Sep 15, 2018

Hello, thanks for an awesome framework!
This PR adds support for reloading NSCollectionView and NSTableView on macOS.
I've renamed and reorganized the existing UIExtensions to Extensions with categories for respective class.

gif

Copy link
Owner

@ra1028 ra1028 left a comment

Choose a reason for hiding this comment

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

@OskarGroth
Looks great!
Please check the some comments.

@@ -24,7 +24,10 @@
6B5B40AA211066EA00A931DB /* ArraySection.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B5B408A211066B300A931DB /* ArraySection.swift */; };
6B5B40AB211066EA00A931DB /* AnyDifferentiable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B5B408B211066B300A931DB /* AnyDifferentiable.swift */; };
6B5B40AC211066EA00A931DB /* ElementPath.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B5B408D211066B300A931DB /* ElementPath.swift */; };
6B956B762110B25300DE3D29 /* UIKitExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B5B4088211066B300A931DB /* UIKitExtension.swift */; };
75C6E50A214D66F80016493B /* UITableView+Reload.swift in Sources */ = {isa = PBXBuildFile; fileRef = 75C6E509214D66F80016493B /* UITableView+Reload.swift */; };
Copy link
Owner

@ra1028 ra1028 Sep 17, 2018

Choose a reason for hiding this comment

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

I prefer that AppKitExtension and UIKitExtension to the file names.

//
// Created by Oskar Groth on 2018-09-15.
// Copyright © 2018 Ryo Aoyama. All rights reserved.
//
Copy link
Owner

Choose a reason for hiding this comment

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

[Nit] Please remove all the header comments.


@available(OSXApplicationExtension 10.11, *)
public extension NSCollectionView {

Copy link
Owner

Choose a reason for hiding this comment

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

[Nit] Please remove unneeded empty line.

endUpdates()
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

ditto

@@ -33,7 +41,7 @@ public extension UITableView {
setData: setData
)
}

Copy link
Owner

Choose a reason for hiding this comment

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

[Nit] Please revert all the white space changes.

@ra1028
Copy link
Owner

ra1028 commented Sep 17, 2018

Could you add a simple example app?

@OskarGroth
Copy link
Contributor Author

Cleaned up PR and added example project, see gif at top 😄

Copy link
Owner

@ra1028 ra1028 left a comment

Choose a reason for hiding this comment

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

@OskarGroth
Wow, This is awesome! Thaks.

@ra1028 ra1028 merged commit 99a40fa into ra1028:master Sep 19, 2018
@ra1028 ra1028 mentioned this pull request Sep 19, 2018
@dispatchqueue
Copy link

@OskarGroth @ra1028 I'm having an issue with the sample. When you click Shuffle button, the data you see in the table view is not same as the collection view although they are using the same array:

image

The collection view works well. If you add this code after shuffling the array, the table view will be reloaded and show correct data after 1s:

dataInput.shuffle()

DispatchQueue.main.asyncAfter(deadline: .now() + 1) {
    self.tableView.reloadData()
    self.collectionView.reloadData()
}

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.

3 participants