codernaut / citizenconnect Goto Github PK

I fetched recent changes and now can't build because of recent changes regarding SendEmail functionality.

I can see that 3 external libs were added in build.gradle file

    implementation files('/Development/Code for Pakistan/citizenconnect/Android App/libs/activation.jar')
    implementation files('/Development/Code for Pakistan/citizenconnect/Android App/libs/additionnal.jar')
    implementation files('/Development/Code for Pakistan/citizenconnect/Android App/libs/mail.jar')

But there were two problems with this approach:

1. They are absolute paths which means every contributor is bound to follow it which is not possible. It should have been like implementation files('libs/mail.jar')
2. The libs folder along with the jar files was not pushed to the repo or at least I don't see it which is the main reason I can't build it.

Something unrelated to the fix but I can see that there is a feedback form where user can enter message for feedback/question and an email is sent to [email protected]. I can also see that the code was used from somewhere like this tutorial. No harm in copying code if someone has already shared a solution to your problem but from how I can see this is a lot of unnecessary code with a lot of code smell. I tried to find a better way to silently send email but seems like Android didn't introduce a native way to do it through its own APIs. Maybe there is a reason why it is like this.

Honestly, I don't understand the need for this hassle of creating separate utility classes and adding external libs which should be the max last resort and not so recommended. It could have been dealt in a very easy way with allowing user to send Email by only showing the email clients to choose and pre-filling the recipient, subject and even body field where user can further add his/her message and just click send.

What are your thoughts? Was there a very critical reason for such feature to programmatically send feedback?

Quoting Gulzaib's response in the email: update readme with required node modules.node js version etc because removed them from repo

I don't know about the official description so probably we need help from Sidra.

I have noticed that text is directly quoted in classes which is a very bad practise. Strings should always be first created in strings.xml and then this resources should be references wherever needed.

See examples related to progressDialog in PhoneVerificationActivity
progressDialog.setMessage("Verifying...");

I introduced these two new strings in last PR and tried to replace wherever "Please wait..." was used.

<string name="in_progress_msg">Please wait…</string>
<string name="completed_msg">Operation Completed!</string>

Also better if a certain naming convention is used in strings.xml for naming resources. I can guide if there is any confusion with names.

But I would suggest if @shahzaib414 can also replace the newly introduced strings in Release Demo PR

Some type of images looks ugly on the notification list because they stretch. It's not a big of a thing. I started it already and will present something soon.

notification json objects stored in firebase are not compatible with defined standard

I can see a lot of unnecessary files which could be avoided if there is a proper .gitignore file in use.

For starting, we can use it from one of my projects https://github.com/wahibhaq/urdu-font-comparator-app/blob/master/.gitignore and further improve it if needed.

we need an endpoint which can be called using postman or custom http client in any desktop platform for pushing notification

When we install app for first time all the data sets are loaded, Though its done in a separate asynch tasks the spalsh screen becomes unresponsive. Investigate and fix

I will keep adding more items as I found some things to improve and later we can decide if it can be done in one PR or separate PRs.

• Integrate buildTypes { debug { } release { } }
• integrate android { signingConfigs { debug { } release { } } }

Splash screen shows up on first install and works fine. But whenever the installed app is killed and restarted, you see a blank white screen for lets say 5-6 seconds which is not a good user-experience. Maybe we should show the similar splash screen again because app tries to fetch data on app start anyways so it should be visible to user that some processing is happening in background.

I would understand if you guys had plans to involve Dagger some point in near future as it was a beginning but my experience tells me that earlier is even better.

Dagger make it easy to create reusable, interchangeable modules, offers better resource sharing and facilitates testable code. I can help in kicking-off with it. It will provide the support on which team can build a stable and scalable product.

1. I can see that RxJava dependency is included in gradle compile 'io.reactivex:rxjava:1.1.0' but only referred in the 3 classes in io.reactivex package which is also then not used. Removing unused code is always better. Also I would highly recommend to use it when needed but RxJava2 instead of RxJava1 which is already obsolete.

2. Picasso dependency is also unused if I am not wrong.

I would strongly suggest to make app only support portrait because of 2 reasons.

1. I don't see much use with landscape mode. This is a typical content (mostly lists based screen) heavy app which works perfectly with portrait mode
2. We can save ourselves from a lot of problems related to retaining state when screen is rotated. State Management in Android is not an easy thing. There are solutions possible of course but always adds an additional overhead. Considering our situation its efficient to reduce potential problems.

P.S: Currently app crashes on Services tab and you try to move from portrait -> landscape -> come back to portrait which is caused by NullPointerException in MainActivity:122. We should definitely fix this problem but also disable landscape mode.

I can handle it or even shahzaib can handle it. not a big deal!

I have the latest code from codernaut:master and after fixing the libs/mail.jar issue I got another build error because this icon doesn't exist in drawable but it is referred in MainActivity.java:69.

AHBottomNavigationItem item2 = new AHBottomNavigationItem(R.string.bottom_item2, R.drawable.ic_documents_filled, R.color.colorPrimary);

In the production app, I can see that this is a Services tab but can't find the icon used.

@shahzaib414 did you push it? I went through the code in this repo and also couldn't find.

Its a widely known best practice among android folks to avoid hardcoding version numbers of dependencies inside app/build.gradle. Instead its recommended to store it either in root build.gradle or in the same gradle file but as variables and then refer these variables.

It makes it much easier to update because then you can reuse one variable in different dependency and also easier to read for other contributors.

For example in root build.gradle

supportLib = '27.0.2'

and in app/build.gradle

// App's support dependencies
  implementation "com.android.support:appcompat-v7:$rootProject.supportLib"
  implementation "com.android.support:support-v4:$rootProject.supportLib"