Skip to content

ImmuTable#4547

Merged
koke merged 18 commits intodevelopfrom
feature/immutable
Dec 10, 2015
Merged

ImmuTable#4547
koke merged 18 commits intodevelopfrom
feature/immutable

Conversation

@koke
Copy link
Copy Markdown
Member

@koke koke commented Dec 4, 2015

I think if you are building a view that's just a table that's really kind of a form, your view controller should only have to specify those cells in a declarative way. The only code there should be how to get a model, turn it into a view model, and handle any changes.

ImmuTable was already introduced in #4510 but I'm extracting it to a separate PR for easier review.

If you want to test it and see it in action, I've reimplemented Settings with ImmuTable in #4549

Needs Review: @jleandroperez

This was referenced Dec 4, 2015
@koke koke added this to the 5.9 milestone Dec 4, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just wondering if we'd also want support for NSAttributedStrings here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think for most cases I can think of, a plain String will be enough. If it becomes necessary we can add it later

@jleandroperez
Copy link
Copy Markdown
Contributor

@koke Code looks great! but it's honestly hard to follow the entire hierarchy. (Edit: solvable with some docs!)

Plus, i've got some doubts, such as:

  • If you wanna have (any of the standard cells), with a custom accessory indicator, you'd need to just implement another cell, with the style you want. Just wondering if we wanna extend the Row struct to hold more (optional) properties.
  • Maybe there's a simpler way (aka less code) to deal with the MockTableView concept. (OCMock?). That'd help eliminate a couple protocols.
  • Just wondering about the naming consistency detail. ImmuTable <> WPImmuTableCells (when do we want the WP prefix, and when we don't).

@koke
Copy link
Copy Markdown
Member Author

koke commented Dec 8, 2015

If you wanna have (any of the standard cells), with a custom accessory indicator, you'd need to just implement another cell, with the style you want. Just wondering if we wanna extend the Row struct to hold more (optional) properties.

You'd need to implement another Row, as long as your cell isn't a custom class. As for extending Row, remember that ImmuTableRow is a protocol, not a base class, so any properties added there would still need to be declared in every type that implements the protocol. We wouldn't have less code, and we'd have many more nils floating around.

Just wondering about the naming consistency detail. ImmuTable <> WPImmuTableCells (when do we want the WP prefix, and when we don't)

I added the prefix because they inherit from WPTableViewCell. I'd love to get rid of that dependency and have ImmuTableCells with a sensible default set of cells.

@koke
Copy link
Copy Markdown
Member Author

koke commented Dec 8, 2015

Maybe there's a simpler way (aka less code) to deal with the MockTableView concept. (OCMock?). That'd help eliminate a couple protocols.

Maybe there's a shorter way, but I'm not sure if that'd make it better or more readable

koke added 7 commits December 8, 2015 15:23
Several changes to make the API simpler:

- Only one row protocol: dropped CustomImmuTableRow,
  CustomCellImmuTableRow, and CustomNibImmuTableRow in favor of just
  ImmuTableRow.
- Moved all the cell class and reusable identifier data and logic inside
  the registrable enum.
- Renamed CellRegistrable to ImmuTableCell for clarity.
- Provided a default (nil) implementation for customHeight
@koke
Copy link
Copy Markdown
Member Author

koke commented Dec 8, 2015

@jleandroperez ready for round two, I'm quite happy with this iteration as everything looks much simpler.

I'm going to keep adding some more documentation, but feel free to start with the code review

@koke
Copy link
Copy Markdown
Member Author

koke commented Dec 8, 2015

I'm done with documentation as well, and reorganised the file to make it more approachable. Please let me know if you find the docs easy to understand.

After this, I'd like to try to move this to a separate library with a bunch of default cells, so it might help with documentation to have a few examples. Also a README separate from the header docs could really help explain what this is and how you're supposed to use it. I added a bit of that at the top of the file, but there's a lot of room for improvement.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 Looking nice! (Both, unowed, and a single Handler object!)

@jleandroperez
Copy link
Copy Markdown
Contributor

@koke i really like what you did with ImmuTable!.

I meant to answer this one, but Github doesn't show the Q anymore:

extension UITableViewController: ViewControllerWithTable { }

Latest implementation looks better than that (less complicated!). There's always room to extend / improve (support for UIViewControllers, ie), but i'm sure ImmuTable can grow as needed / when needed / if needed.

:shipit: !!

koke added a commit that referenced this pull request Dec 10, 2015
@koke koke merged commit 402e778 into develop Dec 10, 2015
@koke koke deleted the feature/immutable branch December 10, 2015 07:15
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