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

Two issues solved: #46

Merged
merged 1 commit into from
Oct 4, 2016
Merged

Two issues solved: #46

merged 1 commit into from
Oct 4, 2016

Conversation

vikiCoder
Copy link
Contributor

1.)Select all while selecting webmails
2.)Save selected webmails in inbox and show back in onResume

1.)Select all while selecting webmails
2.)Save selected webmails in inbox and show back in onResume
@crearo
Copy link
Owner

crearo commented Sep 12, 2016

This looks great. I'll go through this thoroughly tomorrow, and if everything's alright, I'll merge asap :)

@vikiCoder
Copy link
Contributor Author

Hello,
I hope that you have seen the changes that I've made. Actually I am new to github and opensource, so please tell and guid me if I've done something wrong so that I can correct it.

@crearo
Copy link
Owner

crearo commented Sep 20, 2016

Hey,
I had a look at the code, and it all looks great.

  1. You've followed all conventions - nomenclature, method names. access modifiers of variables, etc
  2. You've followed appropriate guidelines by Android on restoring instance states - which can be slightly tricky at times.
    So, great job!

There are a couple of minor changes that I'll make - for example the switch case in the MainActivity where all strings are referenced using "quotes" - instead, reference strings by creating them in Constants.java in the util package. That way, silly errors like spelling mistakes won't get in the way of development. Apart from that, all is fine from your side. :)
I'm really busy this week, and will merge the pull request this weekend. 👍

Also, till then, since you've had a fair look at the code and know how everything is working, do you think you could work on a couple of the other issues? Pull up to load more issue here is something lots of users have been writing to me about. If you do work on it, I suggest you to create a new branch in your repository with the name load-more and work on that branch.

For next time, remember when sending a pull request, work on a separate branch, and keep your master branch clean (up to date with the repo you fork from). This is because if you make any changes to your master branch now, they will show up here on this pull request.

@vikiCoder
Copy link
Contributor Author

Ok I will definitely try other issues.

@crearo
Copy link
Owner

crearo commented Sep 28, 2016

Hey, I'm really sorry I'm taking this much time to merge your request. I've been super busy of late.
I'll definitely do so this weekend. Thanks for your patience!

@crearo crearo merged commit 3c0ba38 into crearo:master Oct 4, 2016
@crearo
Copy link
Owner

crearo commented Oct 4, 2016

Great work :)

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