daniel-stoneuk / material-about-library

Makes it easy to create beautiful about screens for your apps
Apache License 2.0
1.12k stars 140 forks source link

Are you interested in merging my changes? #8

Closed ueman closed 7 years ago

ueman commented 7 years ago

I've added the following things:

I would tidy up my code a little bit and would then open a pull request. You can take a look at my changes here: https://github.com/ueman/material-about-library/

daniel-stoneuk commented 7 years ago

Your code looks interesting! If I'm honest, I wouldn't want to merge all of it but I've just spent some time looking through your code and it looks great.

I'm not sure about starting the about page with only an intent, I decided to go for extending an abstract class as it allows for more functionality and because it's not possible (as far as I'm aware) to pass in the MaterialAboutList - I've tried parceling but obviously that didn't work out.

I however, love the idea of the ConvenienceBuilder class. I would like to merge that but don't think bundling icons with the library is a good idea. Maybe you could make the builders less specific (eg remove the facebook one but leave the website one).

I'm sorry if I'm being a bit picky and I hope you're not offended or anything! I really do appreciate the work you've done on the library.

You could open a pull request and then I can refactor the code to only include the things I would like to implement?

ueman commented 7 years ago

The IntentBuilder for the AboutPage was intended as an addon for even simpler creation of an AboutPage. Yeah, I'm not offended or anything. I'm doing a Pull Request then and you can do whatever you want with it.

daniel-stoneuk commented 7 years ago

I'm going to be slightly busy over the next couple days, so will merge this weekend. Thanks again!

ueman commented 7 years ago

You could also look into this commit on how to simplify you code a little bit and on how to remove those instanceof if-else things.