search

Tapadoo / Alerter

An Android Alerting Library
MIT License
5.52k stars 633 forks source link

feature : Alerter for Dialog #249

Closed ShwetaChauhan18 closed 3 years ago

ShwetaChauhan18 commented 3 years ago

Solution for this issue : https://github.com/Tapadoo/Alerter/issues/247

I need this feature in urgent base, So if it's good if you review and check this feature. Otherwise I have to add module in my project.

Also, I can refactor code in next PR.

kpmmmurphy commented 3 years ago

Hey @ShwetaChauhan18, thanks for your PR, and great to see an implementation for this! There is a lot of code duplication in the AlerterForDialog class, which appears to be directly copied from the original Alerter class. I think an inheritance approach would be better, as it would reduce this duplication. I'm not happy to merge this in at the moment, so I do recommend building your fork locally and including it as a module. I'll provide a few comments on the changed files and we can take it from there! Thanks again.

ShwetaChauhan18 commented 3 years ago

Hey @ShwetaChauhan18, thanks for your PR, and great to see an implementation for this! There is a lot of code duplication in the AlerterForDialog class, which appears to be directly copied from the original Alerter class. I think an inheritance approach would be better, as it would reduce this duplication. I'm not happy to merge this in at the moment, so I do recommend building your fork locally and including it as a module. I'll provide a few comments on the changed files and we can take it from there! Thanks again.

changed

ShwetaChauhan18 commented 3 years ago

@kpmmmurphy : Please Review. I have refactored all code.

kpmmmurphy commented 3 years ago

Thanks for updating your PR @ShwetaChauhan18, we're definitely moving in the right direction. I think if you refactor the Alerter to only maintain a reference to the DecorView, it will be ready to merge! Don't forget to update the version in the build.gradle, and the changelog! I think it's best to move to version 7.0.0. Thanks again.

ShwetaChauhan18 commented 3 years ago

@kpmmmurphy : PR Updated as per comment. Please Review it and Thanks a lot for quick response.

kpmmmurphy commented 3 years ago

Hey @ShwetaChauhan18, I've created a new PR (#251) based of your code here. I refactored to improve the API, creating a cleaner calling surface. Thanks again for your help. I'm going to close this one out. One important thing to note, is that force pushing clears the PRs history, making it hard for reviewers to spot code changes for different commits - in future I'd recommend trying to maintain the commit history, as it makes reviewing easier! 🙌