-
Notifications
You must be signed in to change notification settings - Fork 7
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
Sample app #25
Sample app #25
Conversation
d89727b
to
3d1a71a
Compare
1927f16
to
c6660dd
Compare
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.
Couple of nitpicks! Great to see the example ❤️
@@ -0,0 +1,26 @@ | |||
package com.example.vangogh; |
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.
I would rather use the same base package name (also application ID) as the library itself, i.e. com.pspdfkit.labs.vangogh.*
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
|
||
import static org.junit.Assert.*; |
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.
Nit: Favor single imports instead of wildcard ones.
* @see <a href="http://d.android.com/tools/testing">Testing documentation</a> | ||
*/ | ||
@RunWith(AndroidJUnit4.class) | ||
public class ExampleInstrumentedTest { |
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.
You can drop the boilerplate test.
@NonNull | ||
public List<MainMenuItem> getMainMenuItems() { | ||
List<MainMenuItem> items = new ArrayList<>(); | ||
items.add(new MainMenuItem("Fade animation", "Showcases the fade in/out animation", new SingleFadeAnimationActivity())); |
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 do you create "fake instances" of the example activities here? Wouldn't it be simpler to just attach the class of each test activity to the menu item instead, i.e. SingleFadeAnimationActivity.class
?
|
||
@NonNull | ||
@Override | ||
public View getView(int position, @Nullable View convertView, @NonNull ViewGroup parent) { |
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.
Variables in here can be marked final
.
|
||
public class MainMenuAdapter extends ArrayAdapter<MainMenuItem> { | ||
|
||
private OnMainMenuItemClickListener listener; |
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.
Should be marked @Nullable
.
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.
Or maybe inject a @NonNull
listener through the adapter's constructor? It's not necessary in this setup that the listener is optional.
* | ||
* @see <a href="http://d.android.com/tools/testing">Testing documentation</a> | ||
*/ | ||
public class ExampleUnitTest { |
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.
Dummy unit test can be removed.
Resolves #19
Add sample app that uses VanGogh library to showcase some sample animations.
In particular, adds rotate and fade in/out animations for the beginning. In the future, more complex animations should be showcased: #26
Animations are triggered by clicking the button that will be animated (the only button on the screen).
Rotate animation sample
Fade animation sample