-
Notifications
You must be signed in to change notification settings - Fork 73
Added App information in about section #350
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @lakshyagupta21 suggested use customTab as this don't use the default one
and Add some GIFs for the same so, people can review easily
@@ -74,4 +74,5 @@ dependencies { | |||
// Permissions Dispatcher | |||
implementation "org.permissionsdispatcher:permissionsdispatcher:4.5.0" | |||
annotationProcessor "org.permissionsdispatcher:permissionsdispatcher-processor:4.5.0" | |||
implementation 'androidx.browser:browser:1.2.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why introducing a third-party lib for this small feature @devanshi7799 ? If I were you, I will implement that using a simple web view.
Downsides for introducing a new lib in this case are:
- Some third-party libs have many redundant functions, that will increase the apk size.
- Some third-party libs are not well tested that may introduce bugs that cannot fix.
So we are trying not to use third-party libs unless we have to do that, I think this would be helpful for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why introducing a third-party lib for this small feature @devanshi7799 ? If I were you, I will implement that using a simple web view.
Hi @huangyz0918, it's a custom tabs dependency, and nowadays every app has in-app browser feature which provides a look and feel of the browser within the app without actually moving out of the app, so I think its fine we even have this in ODK-Collect but not AndroidX dependency. I think @devanshi7799 this is fine no need to explore the webview as of now, if any feature ask comes in future then we can pick that up or if it we face any issues due to this then we can pick this up.
@huangyz0918 should i do it using a webview? |
Yes, you should have a try. |
public void onItemClick(View view, int position) { | ||
if (position == 0) { | ||
|
||
new CustomTabsIntent.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern was that what if the device doesn't support custom tabs, it would be easier for you to understand if you can explore ODK-Collect Custom tabs implementation that they have because that handles lot of things which we might not be able to test on our devices, there are even few devices which doesn't have browser in them, so this change may crash the app on those devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lakshyagupta21 Should i make changes according to odk-collect custom tabs?
Closes #175
What has been done to verify that this works as intended?
I ran the application on my phone and it worked perfectly fine
Why is this the best possible solution? Were any other approaches considered?
Here, I have added a new html file for app information in the assets and made some changes in AboutActivity.java
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
New html page is added regarding the information about the application as it was expected
Before submitting this PR, please make sure you have:
./gradlew checkCode
and confirmed all checks still pass OR confirm CircleCI build passes