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

Custom style #69

Merged
merged 14 commits into from
Jan 29, 2021
Merged

Conversation

mattcroxson
Copy link

The purpose of this PR is to provide users with the capacity to expose the default table view style with any valid UITableView.Style option, and to provide a custom title for the presented view controller:

  • UITableViewController accepts a UITableView.Style property in one of it's initialisers. This change allows a user to override the default .grouped with either .plain or .insetGrouped (available on iOS/iPad OS 13.0 +) to match their app's UI design when instantiating a new AcknowListViewController
  • In some instances users may wish to override the default localized title "Acknowledgements" with a custom title. This change allows users to do so when instantiating a new AcknowListViewController

@vtourraine
Copy link
Owner

Hello Matt, and thanks a lot for preparing this pull request 👍

I appreciate that you want to tailor the library to your app’s needs, but I try to limit the number of options built into the library itself, while keeping the door open for client-specific customizability.

For the custom title, I think it’s better to just set the UIViewController.title property. For the table view style, you can subclass the controller or just call a different initializer.

Keeping the number of options to a minimum really helps with code maintainability and accessibility. I hope you understand my reasoning.

@ApolloZhu
Copy link

I agree on setting custom title using the UIViewController property, but I'm not sure if the subclassing option would work for setting a different table view style, since there are no initializer that takes a style parameter:

there's no available initializer that takes a custom table view style

@vtourraine
Copy link
Owner

@ApolloZhu Ah, you’re right, I was thinking of essentially calling super.super.init(), but that’s just not possible. 😅

As it happens, another pull request just got opened with the same request: #70. So I’m starting to reconsider the “table view style” parameter.

@Lumus, would you like to update your branch to remove the customTitle changes, and only keep the table view style additions? Thanks!

@mattcroxson
Copy link
Author

@Lumus, would you like to update your branch to remove the customTitle changes, and only keep the table view style additions? Thanks!

All good. It hadn't even crossed my mind to set the title externally HAHA. Will update the PR shortly

@vtourraine
Copy link
Owner

@Lumus Thank you for the follow-up changes (and for updating the unit tests, documentation, and example app too 👏👏👏).

I’ve updated your branch to tighten the code a bit. The main change is that I’ve reverted the convenience initializers of AcknowListViewController to not include the style parameter. My reasoning is that, again, I want to limit the number of options and API “surface”. This keeps the library as easy-to-use as possible, while keeping the door open to customization.

What do you think?

@mattcroxson
Copy link
Author

@Lumus Thank you for the follow-up changes (and for updating the unit tests, documentation, and example app too 👏👏👏).

I’ve updated your branch to tighten the code a bit. The main change is that I’ve reverted the convenience initializers of AcknowListViewController to not include the style parameter. My reasoning is that, again, I want to limit the number of options and API “surface”. This keeps the library as easy-to-use as possible, while keeping the door open to customization.

What do you think?

@vtourraine 👍 I have no issues with the changes you made. I'll admit I have a tendency towards "give the user as many options as possible" but that can come at the cost of too much complexity hehe. In this though limiting the options on the API "surface" will be beneficial. 😄

@vtourraine vtourraine changed the title Custom title and style Custom style Jan 29, 2021
@vtourraine vtourraine merged commit 211948b into vtourraine:master Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants