-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Support dynamic tables #42
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #42 +/- ##
=========================================
+ Coverage 92.59% 95.1% +2.5%
=========================================
Files 15 14 -1
Lines 378 347 -31
=========================================
- Hits 350 330 -20
+ Misses 28 17 -11
|
Hi @twodayslate, thanks for opening this pull request. Can you briefly explain what these changes are for? |
@bcylin I included an example to demonstrate the capabilities. The implementation is rather simplistic but it allows for the use of I would think this fixes the main limitation of Open to suggestions and improvements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twodayslate thanks for improving this library.
The demo you added in the example looks nice! However, it doesn't seem to require any changes in the QuickTableViewController
class if I don't miss anything. Also, this library was not designed for adding and removing rows in the table view. With the trend towards SwiftUI, I think that'd be a much better solution to build such dynamic table contents.
I tend not to add unnecessary workarounds to the library. If you'd like to keep the demo in the example, can you please revert the changes in the Source
directory? Thanks much!
internal final class DynamicViewController: QuickTableViewController { | ||
|
||
var dynamicRows: [Row] = [] | ||
|
||
override var tableContents: [Section] { | ||
get { | ||
let rows = [ | ||
TapActionRow(text: "AddCell", action: { _ in | ||
self.dynamicRows.append( | ||
NavigationRow(text: "UITableViewCell", detailText: .value1(String(describing: (self.dynamicRows.count + 1))), action: nil) | ||
) | ||
self.tableView.insertRows(at: [IndexPath(row: self.dynamicRows.count, section: 0)], with: .automatic) | ||
}) | ||
] + dynamicRows | ||
|
||
return [ | ||
Section(title: "Tap Action", rows: rows as! [Row & RowStyle]) | ||
] | ||
} | ||
set {} // swiftlint:disable:this unused_setter_value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you revert all the changes in the Source
directory, these lines will still make the demo work. It seems you don't need to modify QuickTableViewController
or introduce a new delegate at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculations of tableContents
wouldn't be cached tho. Add a sleep(1)
and/or print
to the tableContents
getter and the performance difference/access count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need cache due to the dynamic contents, please implement the cachedTableContents
in the subclass and override tableView
in the DynamicViewController
, e.g. 9dec58a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me. I assume you have no desire to actually put QuickTableView
and QuickTableViewDelegate
into the package? And/or perhaps a QuickDynamicTableViewController
which does the subclassing in the example for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dynamic table view controller in the example would be nice. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying out new ideas! Just a few minor tweaks before merging. LGTM 👍
Thank you for taking the time to review this. Your feedback was appreciated. |
No description provided.