vtourraine / AcknowList

Acknowledgements screen displaying a list of licenses, for example from CocoaPods and Swift Package Manager dependencies.
MIT License
798 stars 60 forks source link

Custom style #69

Closed mattcroxson closed 3 years ago

mattcroxson commented 3 years ago

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:

vtourraine commented 3 years ago

Hello Matt, and thanks a lot for preparing this pull request :+1:

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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. 😄