Add spritz library and demo #32

frapontillo · 2017-09-21T09:40:22Z

Spritz is a new library for seamlessly animating a Lottie view by swiping in a ViewPager.

Documentation

The root directory of the project has a README that explains how to use the library with a detailed list of all methods. The library doesn't have a Javadoc yet.

GIF

The outcome is shown in the demo app:

Next steps

As soon as this library is merged on master, I'll add the publish plugin so we can release it as an open source library on maven.

mr-archano

I only glanced at the API and setup, will spend sometime later to look into the details, but I left some initial remarks.

mr-archano · 2017-09-21T09:46:40Z

settings.gradle

 include 'MapApiV2Demo'
 include 'MultipleContacts'
 include 'OptionalDependencies'
 include 'PinchZoomDetector'
 include 'simperAudioStreamer'
 include 'sms'
+include 'spritz'


Please let's not do this. There's literally no reason to include this in the root project. It's an anti-pattern coming from historical reasons, that we should not adopt going forward (happens the same with the dojo repo).

I was merely adhering to what I read in there 😂
Done in 96a580d!

mr-archano · 2017-09-21T09:48:26Z

spritz/app/proguard-rules.pro

@@ -0,0 +1,25 @@
+# Add project specific ProGuard rules here.


Do we need this file?

❌ in 4bfb165.

mr-archano · 2017-09-21T09:50:05Z

spritz/spritz-android/proguard-rules.pro

@@ -0,0 +1,25 @@
+# Add project specific ProGuard rules here.


Lot of leftover from the IDE default wizard. Probably good idea to clean up.

❌ in 4bfb165.

mr-archano · 2017-09-21T09:53:32Z

spritz/spritz-android/src/main/java/com/novoda/spritz/Spritz.java

+            return this;
+        }
+
+        public Spritz attachTo(ViewPager viewPager) {


I'm not a fan of this, it seems like we're trying to emphasise on fluent API missing flexibility and explicitness. I'd expect a Spritz instance to implement ViewPager.OnPageChangeListener and let the final user decide when to attach or detach it. (The latter case is not considered at all)

I'm actually ok with this being the point where you attach to a viewPager, but then we need a detach() method on Spritz itself as Toto suggests.

Sorry I wasn't clear: I meant I'm not a fan to be built-in the builder. It should be part of the Spritz instance API imo.

Yeah I understood that, I meant I don't have a problem with that to be honest

So @mr-archano what would be your idea for this API to be?

I think what @mr-archano means is removing attachTo from the builder API and add it to the Spritz object returned by the builder API.
So you'll do:

Spritz sp = Spritz .with(lottieAnimationView) .withDefaultSwipeAnimationDuration(300, TimeUnit.MILLISECONDS) .withDefaultSwipeForwardInterpolator(SWIPE_FORWARD_INTERPOLATOR) .withSteps(...) .build(); // Later on sp.attachTo(viewPager);

Or maybe he meant returning a ViewPager.OnPageChangeListener and let the user setting it on the ViewPager?

mr-archano · 2017-09-21T09:54:19Z

spritz/spritz-android/src/main/java/com/novoda/spritz/SpritzPage.java

+
+import java.util.concurrent.TimeUnit;
+
+public class SpritzPage {


This reads "page" but it just contains only info about animation :/

Maybe Step is clearer and clashes less with the ViewPager's Page domain?

Done in e2a51cd! 💯

rock3r

Nice library! Few comments to address tho.

rock3r · 2017-09-21T10:01:56Z

spritz/.gitignore

@@ -0,0 +1,9 @@
+*.iml


May I suggest using a more thorough .gitignore? Like one from client projects, or Squanchy's.

Shamelessly copied from squanchy: 62ad738

rock3r · 2017-09-21T10:07:22Z

spritz/README.md

+
+After getting your `LottieAnimationView` and your `ViewPager`, just create a `Spritz` object with the following syntax:
+
+```


Can you put the language here so it does syntax highlighting?

```java

Same elsewhere.

Done in 7adc463.

rock3r · 2017-09-21T10:09:10Z

spritz/README.md

+
+After that, you can proceed setting your view pager transitions.
+
+#### `withDefaultSwipeAnimationDuration`


I would rather go with a use-case oriented docs, not with a semi-javadoc-in-the-readme thing like this. E.g., Specify animation durations which would then have some explanation of what it means, and a list of the methods to achieve things.

Shall I do this in my next PR?

Sure ¯\_(ツ)_/¯

rock3r · 2017-09-21T10:12:28Z

spritz/README.md

+#### `withDefaultSwipeBackwardsInterpolator`
+
+`withDefaultSwipeBackwardsInterpolator(TimeInterpolator swipeBackwardsInterpolator)` sets the interpolator to use for all pages to complete the swipe 
+animation if the user stops  dragging backwards at any point. If you don't set this, it will default to `LinearInterpolator`. You can always override 


Double space: stops dragging

Done in 30085d0.

rock3r · 2017-09-21T10:13:28Z

spritz/README.md

+
+#### `withPages`
+
+`withPages(SpritzPage... pages)` lets you specify the number of pages to animate through. The library is made to work with the same number of pages as 


lets you specify the ~~number of~~ pages to animate through

I would also explain better the idea behind the next sentence, something like The number of pages you pass in must match the number of pages on the ViewPager you'll attach Spritz to

Done as part of e2a51cd.

rock3r · 2017-09-21T10:42:05Z

spritz/spritz-android/src/main/java/com/novoda/spritz/Spritz.java

+            return this;
+        }
+
+        public Spritz attachTo(ViewPager viewPager) {


I'm actually ok with this being the point where you attach to a viewPager, but then we need a detach() method on Spritz itself as Toto suggests.

rock3r · 2017-09-21T10:43:16Z

spritz/spritz-android/src/main/java/com/novoda/spritz/SpritzPage.java

+
+import java.util.concurrent.TimeUnit;
+
+public class SpritzPage {


Maybe Step is clearer and clashes less with the ViewPager's Page domain?

rock3r · 2017-09-21T10:44:33Z

spritz/spritz-android/src/main/java/com/novoda/spritz/SpritzPageWithOffset.java

+        return swipeBackwardsInterpolator;
+    }
+
+    static List<SpritzPageWithOffset> fromSpritzPages(SpritzPage... spritzPages) {


Not sure this should be here, I am not a fan of a (List<ModelA>) -> List<ModelB> sitting in ModelB. Would move to another class.

rock3r · 2017-09-21T10:45:43Z

spritz/spritz-android/build.gradle

+    compile 'com.android.support:design:26.1.0'
+    compile 'com.airbnb.android:lottie:2.1.0'
+
+    testCompile 'junit:junit:4.12'


This is unused too as far as I can tell

rock3r · 2017-09-21T10:46:01Z

spritz/spritz-android/build.gradle

+}
+
+dependencies {
+    compile 'com.android.support:appcompat-v7:26.1.0'


I think both the support dependencies here are unused

Done in f44b30d.

rock3r · 2017-09-22T07:41:53Z

Nice! Only #32 (comment) and #32 (comment) left to address from me

frapontillo · 2017-09-22T08:35:45Z

Comments by @rock3r addressed, waiting for @mr-archano for better API ideas.

lgvalle

A general comment on naming for the builder API.
Why so many withXXX ?

From my point of view the "with" semantic is implicit the moment you call a method.

Why not removing most of them?

Spritz
    .with(lottieAnimationView)
    .defaultSwipeAnimationDuration(300, TimeUnit.MILLISECONDS)
    .defaultSwipeForwardInterpolator(SWIPE_FORWARD_INTERPOLATOR)
    .steps(
            new SpritzStep.Builder()
                    .withAutoPlayDuration(500, TimeUnit.MILLISECONDS)
                    .withSwipeDuration(500, TimeUnit.MILLISECONDS)
                    .build(),
            new SpritzStep.Builder()
                    .withAutoPlayDuration(500, TimeUnit.MILLISECONDS)
                    .withSwipeDuration(500, TimeUnit.MILLISECONDS)
                    .swipeBackwardsInterpolator(SWIPE_BACKWARDS_INTERPOLATOR)
                    .build(),
            new SpritzStep.Builder()
                    .withAutoPlayDuration(500, TimeUnit.MILLISECONDS)
                    .build()
    )
    .attachTo(viewPager);

rock3r

All my concerns addressed 👍

lgvalle · 2017-09-22T14:49:11Z

spritz/build.gradle

@@ -0,0 +1,23 @@
+// Top-level build file where you can add configuration options common to all sub-projects/modules.


Good to know :D

Removed in b2e8c6d.

lgvalle · 2017-09-22T14:56:38Z

spritz/spritz-android/src/main/java/com/novoda/spritz/Spritz.java

+        viewPager.addOnPageChangeListener(new ViewPager.OnPageChangeListener() {
+
+            @Override
+            public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {


The only real business logic is in this method. Perhaps you can extract stuff and unit test it?

frapontillo · 2017-09-25T13:18:27Z

@mr-archano I slightly changed the API so that you build a Spritz instance first, then you can:

get the view page change listener and attach it/detach it yourself
start pending animations
See d0615f0.

frapontillo · 2017-09-25T13:19:02Z

@lgvalle I can remove the with prefix, but then I would have to remove it from the SpritzStep.Builder too, right?

lgvalle · 2017-09-25T15:06:23Z

@frapontillo I guess so. It's a discussion for a separate PR anyway in my opinion: once this is merged we can get philosophical with naming and improve it 👍

frapontillo · 2017-09-26T12:01:24Z

@mr-archano your comments have been addressed in 97a9682 after our talk IRL.

@rock3r README is now more compact, see 6062a0f.

Thanks to @chris95x8, we have a nice spritz icon that has been added to the demo app and to the README, see 98543eb and 6062a0f.

@lgvalle I would extract the business logic in a following PR so we can properly test it 👍 .

frapontillo added 3 commits September 21, 2017 10:37

Add spritz library and demo

4dd83eb

Fix doc

98b423a

Add GIF to README

8c97f9c

rock3r self-assigned this Sep 21, 2017

rock3r self-requested a review September 21, 2017 09:52

mr-archano suggested changes Sep 21, 2017

View reviewed changes

rock3r suggested changes Sep 21, 2017

View reviewed changes

frapontillo added 16 commits September 21, 2017 15:59

Remove module from root project

96a580d

Remove default proguard-rules.pro files

4bfb165

Rename page to step

e2a51cd

Include better gitignore

62ad738

Add java language to markdown code

7adc463

Remove double spacing

30085d0

Add Apache 2.0 license file

bd30c37

Update Lottie, remove duplicate imports

bc0c033

Set allowBackup to false

bec8bf8

Rename constant, make it private

635fd61

Remove Nullable annotation

efe5086

Remove constraint layout dependency

bf8af39

Remove whiteline

b8aac67

Format strings.xml

ebe99aa

Remove comments

7310506

Remove whiteline and comments

2f5e61a

frapontillo added 3 commits September 22, 2017 09:32

Remove support dependencies

f44b30d

Start pending animations in onStart

0a6abe9

Use View for empty mock layout

2be9a18

lgvalle reviewed Sep 22, 2017

View reviewed changes

rock3r approved these changes Sep 22, 2017

View reviewed changes

lgvalle reviewed Sep 22, 2017

View reviewed changes

frapontillo added 2 commits September 25, 2017 14:16

Build Spritz instance and retrieve listener separately

d0615f0

Remove auto-generated comment

b2e8c6d

lgvalle approved these changes Sep 25, 2017

View reviewed changes

frapontillo added 4 commits September 26, 2017 12:08

Rewrite API to build Spritz, then attach, start animations, detach

97a9682

Change default swipe animation

798587a

Add icons to app

98543eb

Update README with more compact info

6062a0f

mr-archano approved these changes Sep 26, 2017

View reviewed changes

mr-archano merged commit 0404907 into master Sep 26, 2017

mr-archano deleted the spritz branch September 26, 2017 15:32


		import java.util.concurrent.TimeUnit;

		public class SpritzPage {


		After getting your `LottieAnimationView` and your `ViewPager`, just create a `Spritz` object with the following syntax:

		```


		After that, you can proceed setting your view pager transitions.

		#### `withDefaultSwipeAnimationDuration`


		#### `withPages`

		`withPages(SpritzPage... pages)` lets you specify the number of pages to animate through. The library is made to work with the same number of pages as

		@@ -0,0 +1,23 @@
		// Top-level build file where you can add configuration options common to all sub-projects/modules.

Add spritz library and demo #32

Add spritz library and demo #32

Conversation

frapontillo commented Sep 21, 2017

Documentation

GIF

Next steps

mr-archano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgvalle Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r commented Sep 22, 2017

frapontillo commented Sep 22, 2017

lgvalle left a comment

Choose a reason for hiding this comment

rock3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frapontillo commented Sep 25, 2017

frapontillo commented Sep 25, 2017 • edited

lgvalle commented Sep 25, 2017

frapontillo commented Sep 26, 2017

lgvalle Sep 22, 2017 •

edited

frapontillo commented Sep 25, 2017 •

edited