Fix #11: Introduce Oppia splash screen. #60
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is to avoid black or white screen when app is launched and resources are not yet initialized
- Used hyperLog library to store app logs in local file. - Logger class is used to log files in entire application - OppiaApplication class is created to initialize hyperlog
veena14cs
changed the title
veena14cs
changed the title
BenHenning
changed the title
|
@veena14cs can you please extract the logging API introduction to its own pull request? We should keep these pull requests as focused as possible to make them easier to review. |
BenHenning
requested changes
Aug 22, 2019
There was a problem hiding this comment.
Thanks @veena14cs! Took a first pass on it.
Besides moving the logging code to its own PR, I had one question about the imported SVGs: are we using all 4 of them here?
|
One other suggestion: can you please add tests for this new splash activity/fragment? They should be AndroidJUnit tests such that they can run on both Robolectric & Espresso (similar to the HomeActivity's test), and should verify that the transition works as expected. |
|
- Removed unwanted spaces - Removed wait time during transition - Added Javadoc comment - created background image for splashActivity and it is set to theme. - Added animation to SplashTheme styles.
|
@BenHenning PTAL |
|
@veena14cs There's a conflict in a file. :) |
|
I happen to think it is better to introduce Dependency Injection already. @BenHenning Thoughts? |
|
Agreed @droidizer. I'm working on introducing Dagger 2 as part of #4. There will be a PR for that soon. Regarding Kotlin--which files in this PR aren't using Kotlin? They all seem to be. @veena14cs can you please resolve the conflict and update your branch? |
|
Sure, Will check and resolve conflict
…On Sun, Aug 25, 2019 at 8:46 PM Ben Henning ***@***.***> wrote:
Agreed @droidizer <https://github.com/droidizer>. I'm working on
introducing Dagger 2 as part of #4
<#4>. There will be a PR for
that soon. Regarding Kotlin--which files in this PR aren't using Kotlin?
They all seem to be.
@veena14cs <https://github.com/veena14cs> can you please resolve the
conflict and update your branch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60?email_source=notifications&email_token=AD4LXZAYZ5JNMSBUZSJBMATQGNG2NA5CNFSM4IOCA73KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5DFWJY#issuecomment-524704551>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD4LXZE5BCPPGL7CR7B32MDQGNG2NANCNFSM4IOCA73A>
.
|
BenHenning
requested changes
Aug 26, 2019
There was a problem hiding this comment.
Thanks @veena14cs! Generally, the PR looks good to me. I had a few substance comments, but most were nits on style since we're in the process of establishing style guidelines.
|
I forgot to remove it. I was trying some other transition effects.
…On Sun, Aug 25, 2019 at 9:22 PM Ben Henning ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks @veena14cs <https://github.com/veena14cs>! Generally, the PR looks
good to me. I had a few substance comments, but most were nits on style
since we're in the process of establishing style guidelines.
------------------------------
In .idea/misc.xml
<#60 (comment)>:
> @@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<project version="4">
- <component name="ProjectRootManager" version="2" languageLevel="JDK_1_8" project-jdk-name="1.8" project-jdk-type="JavaSDK">
Please revert this change--I notice it keeps happening to me as well, but
I've found one way to potentially fix it that will be checked into my next
PR to resolve #4 <#4>.
------------------------------
In app/src/main/AndroidManifest.xml
<#60 (comment)>:
> </intent-filter>
</activity>
+
+ <activity android:name=".HomeActivity">
I believe this can just be:
<activity android:name=".HomeActivity" />
To shorten things.
------------------------------
In app/src/main/java/org/oppia/app/HomeActivity.kt
<#60 (comment)>:
> @@ -33,7 +33,7 @@ class HomeActivity : AppCompatActivity() {
override fun onFailure(t: Throwable) {
// TODO(BenHenning): Replace this log statement with a clearer logging API.
- Log.e("HomeActivity", "Failed to retrieve user app history", t)
+ Log.e("HomeActivity", "Failed to retrieve user app history = "+ t)
Why make this change? If it's related to the previous logging portion of
this PR, please revert.
------------------------------
In app/build.gradle
<#60 (comment)>:
> @@ -60,6 +60,8 @@ dependencies {
androidTestImplementation "com.google.truth:truth:0.43"
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
+ implementation 'com.hypertrack:hyperlog:0.0.10'
This is no longer necessary for the PR--right? If so, please revert.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashActivity.kt
<#60 (comment)>:
> @@ -0,0 +1,27 @@
+package org.oppia.app.splash
+
+import android.os.Bundle
+import android.view.WindowManager
+import androidx.appcompat.app.AppCompatActivity
+import org.oppia.app.R
+import android.content.res.Configuration
+
+/*
+* This activity acts as a container to SplashFragment
Nit: please keep sentences next to each other & align the *s. Also,
Javadocs begin with /**. Finally, we should describe what the activity
contains, rather than making it related to the fragment (since the fragment
is really just an implementation detail). Suggested change:
/** * An activity that shows a temporary loading page until the app is fully opened. */
Though, if there are no additional details to include that that can be
simplified to:
/** An activity that shows a temporary loading page until the app is fully opened. */
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashActivity.kt
<#60 (comment)>:
> +package org.oppia.app.splash
+
+import android.os.Bundle
+import android.view.WindowManager
+import androidx.appcompat.app.AppCompatActivity
+import org.oppia.app.R
+import android.content.res.Configuration
+
+/*
+* This activity acts as a container to SplashFragment
+* There is a background theme set to this activity to display app logo until the app is initialized
+*/
+class SplashActivity : AppCompatActivity() {
+
+ override fun onCreate(savedInstanceState: Bundle?) {
+ super.onCreate(savedInstanceState)
Although we don't yet have a style guide outlined in the overall
architecture document, let's follow a hybrid of JetBrain's Kotlin style
guide and Google's Java style guide. In this case, per
https://google.github.io/styleguide/javaguide.html#s4.2-block-indentation
let's use 2 spaces for indentation and 4 spaces for continuation (which I
think I configured at the project level for Kotlin--maybe make sure that
you're using the project configuration for Kotlin in your IDE and try
reformatting the code via the IDE).
We can evaluate later whether this is the spacing that we want, though I'm
inclined to try and fit more code on a line rather than increasing
whitespace since it makes it harder to read on smaller screens.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashActivity.kt
<#60 (comment)>:
> +
+/*
+* This activity acts as a container to SplashFragment
+* There is a background theme set to this activity to display app logo until the app is initialized
+*/
+class SplashActivity : AppCompatActivity() {
+
+ override fun onCreate(savedInstanceState: Bundle?) {
+ super.onCreate(savedInstanceState)
+ setContentView(R.layout.activity_splash)
+
+ getWindow().setFlags(WindowManager.LayoutParams.FLAG_FULLSCREEN, WindowManager.LayoutParams.FLAG_FULLSCREEN);
+
+ val fragment = SplashFragment()
+ val transaction = supportFragmentManager.beginTransaction()
+ transaction.replace(R.id.fragment_container, fragment)
Small nit: these four lines can be done in one statement. Let's do that
instead of splitting it up.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashFragment.kt
<#60 (comment)>:
> @@ -0,0 +1,45 @@
+package org.oppia.app.splash
+
+import android.content.Intent
+import android.os.Bundle
+import android.view.LayoutInflater
+import android.view.View
+import android.view.ViewGroup
+import androidx.fragment.app.Fragment
+import org.oppia.app.HomeActivity
+import org.oppia.app.R
+
+/*
+* The SplashFragments navigates to Home page once
This should be phrased in terms of its type, e.g.:
A fragment that navigates to HomeActivity once...
Please update, and format the Javadoc bits like mentioned above in
HomeActivity.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashFragment.kt
<#60 (comment)>:
> +package org.oppia.app.splash
+
+import android.content.Intent
+import android.os.Bundle
+import android.view.LayoutInflater
+import android.view.View
+import android.view.ViewGroup
+import androidx.fragment.app.Fragment
+import org.oppia.app.HomeActivity
+import org.oppia.app.R
+
+/*
+* The SplashFragments navigates to Home page once
+* the app is finished loading completely
+* */
+
Nit: Javadocs should not have a blank space between them and the
class/method/field that they're documenting.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashFragment.kt
<#60 (comment)>:
> +import androidx.fragment.app.Fragment
+import org.oppia.app.HomeActivity
+import org.oppia.app.R
+
+/*
+* The SplashFragments navigates to Home page once
+* the app is finished loading completely
+* */
+
+class SplashFragment : Fragment() {
+
+ override fun onCreate(savedInstanceState: Bundle?) {
+ super.onCreate(savedInstanceState)
+
+ }
+ override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?,
Nit: please add spacing between methods.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashFragment.kt
<#60 (comment)>:
> +import org.oppia.app.R
+
+/*
+* The SplashFragments navigates to Home page once
+* the app is finished loading completely
+* */
+
+class SplashFragment : Fragment() {
+
+ override fun onCreate(savedInstanceState: Bundle?) {
+ super.onCreate(savedInstanceState)
+
+ }
+ override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?,
+ savedInstanceState: Bundle?): View? {
+ // Inflate the layout for this fragment
For this & the next comment (close this activity). The code seems pretty
self-documenting in both places, so these comments seem unnecessary.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashFragment.kt
<#60 (comment)>:
> +import androidx.fragment.app.Fragment
+import org.oppia.app.HomeActivity
+import org.oppia.app.R
+
+/*
+* The SplashFragments navigates to Home page once
+* the app is finished loading completely
+* */
+
+class SplashFragment : Fragment() {
+
+ override fun onCreate(savedInstanceState: Bundle?) {
+ super.onCreate(savedInstanceState)
+
+ }
+ override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?,
Nit on continuation: prefer continuing at the higher syntactical element
(e.g. '(' rather than ',') per
https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break
.
------------------------------
In app/src/main/java/org/oppia/app/splash/SplashFragment.kt
<#60 (comment)>:
> + routeToHomePage();
+
+ return view
+ }
+ private fun routeToHomePage() {
+
+ val intent = Intent(requireContext(), HomeActivity::class.java)
+ startActivity(intent)
+
+ // close this activity
+ activity!!.finish()
+
+ }
+
+}
+
Nit: let's have just one EOF line terminator. Ditto elsewhere.
------------------------------
In app/src/main/res/layout/activity_splash.xml
<#60 (comment)>:
> @@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="utf-8"?>
+<androidx.constraintlayout.widget.ConstraintLayout
I think you mentioned earlier that this was changed away from a
ConstraintLayout, but it seems like it's still one--can we make this a
FrameLayout, instead?
------------------------------
In app/src/main/res/values/dimens.xml
<#60 (comment)>:
> @@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources>
+ <integer name="fade_in_duration">1000</integer>
Please include the time unit in the name, e.g. fade_in_duration_ms since
this appears to be in milliseconds.
------------------------------
In app/src/main/res/values/styles.xml
<#60 (comment)>:
> @@ -5,4 +5,25 @@
<item ***@***.***/colorPrimaryDark</item>
<item ***@***.***/colorAccent</item>
</style>
+
+ <style name="OppiaThemeWithoutActionBar" parent="Theme.AppCompat.Light.NoActionBar">
+ <item ***@***.***/colorPrimary</item>
+ <item ***@***.***/colorPrimaryDark</item>
+ <item ***@***.***/colorAccent</item>
+ </style>
+
+ <style name="SplashScreenTheme" parent="OppiaThemeWithoutActionBar">
+ <item ***@***.***/bg_splash</item>
+ <item ***@***.***/Animation.Activity</item>
+ <item name="android:backgroundDimEnabled">true</item>
Out of curiosity, why do we want to enable this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60?email_source=notifications&email_token=AD4LXZG3VBTDX66AWCHU6ATQGNLBBA5CNFSM4IOCA73KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTR7PA#pullrequestreview-279388092>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD4LXZE4MO3MDOD2HHRNDGTQGNLBBANCNFSM4IOCA73A>
.
|
Changed to framelayout
BenHenning
reviewed
Sep 29, 2019
app/src/sharedTest/java/org/oppia/app/splash/SplashActivityTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/splash/SplashActivityTest.kt
Outdated
Show resolved
Hide resolved
BenHenning
approved these changes
Sep 29, 2019
There was a problem hiding this comment.
Thanks @veena14cs. Everything looks fine here, but I'd like you to resolve the open comment thread before merging. Please follow the format I suggested where you use an inline comment like /* paramName= */ next to literal values, and document why we chose those values in single line, line-wrapped, comments above the field.
BenHenning
approved these changes
Oct 1, 2019
There was a problem hiding this comment.
Thanks @veena14cs. Please apply the two code snippets and resolve the conflict, and then this is good to submit.
In the future, I suggest copying suggested code snippets from comments directly into your code, and if they seem like they might not work or don't make sense follow up with a question to help narrow down a solution between you and the reviewer.
app/src/sharedTest/java/org/oppia/app/splash/SplashActivityTest.kt
Outdated
Show resolved
Hide resolved
Thanks @BenHenning will keep this in mind. I have copied these 2 comments and I have I updated code. Its ready to merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Completed
Created splash screen.
Added splash logo.
Check list
Faded transition when loading home page
Handling Configuration changes