Further abstraction #37

leonardo2204 · 2016-12-18T06:08:05Z

As in #34.

leonardo2204 · 2016-12-18T17:46:33Z

Added a very simple example.

zawadz88

Thanks for the PR! Please review the comments I've made. I guess the README file will also need to be updated before we publish these changes to JCenter as well.

zawadz88 · 2016-12-19T12:22:16Z

material-stepper/src/main/java/com/stepstone/stepper/adapter/AbstractFragmentStepAdapter.java

+ */
+public abstract class AbstractFragmentStepAdapter<T extends Fragment & Step> extends FragmentPagerAdapter implements StepAdapter<T> {
+
+    public static final int DEFAULT_NEXT_BUTTON_TEXT = -1;


This should probably be moved to StepAdapter now? The one from AbstractStepAdapter could be removed?

zawadz88 · 2016-12-19T12:23:20Z

material-stepper/src/main/java/com/stepstone/stepper/adapter/StepAdapter.java

+import com.stepstone.stepper.Step;
+
+/**
+ * Created by leonardo on 18/12/16.


Could you add some JavaDoc to the new/changed classes, please?

zawadz88 · 2016-12-19T12:38:45Z

sample/build.gradle

    compile project(':material-stepper')
    compile ("com.android.support:appcompat-v7:$androidSupportLibraryVersion")
    compile ("com.jakewharton:butterknife:$butterknifeVersion")
+    compile 'com.facebook.stetho:stetho:1.4.2'


Could you move the library version to the root build.gradle as the dependencies above?

zawadz88 · 2016-12-19T12:59:14Z

sample/src/main/java/com/stepstone/stepper/sample/nofrag/NoFragAdapter.java

+ * Created by leonardo on 18/12/16.
+ */
+
+public class NoFragAdapter extends AbstractStepAdapter<NoFragView> {


Maybe some of the logic from here could be actually moved to the AbstractStepAdapter?

I thought about it and I can't seem to figure out what else is possible to move up. Any suggestion ?

Maybe the instantiateItem method just as getItem in FragmentPagerAdapter was moved inside of the Fragment version of StepAdapter?
findStep also looks like a good candidate.

In general it might be good to make AbstractStepAdapter be declared as:

public abstract class AbstractStepAdapter<T extends View & Step> extends PagerAdapter implements StepAdapter<T>

similar to how the Fragment version is declared?

zawadz88 · 2016-12-19T13:00:02Z

material-stepper/src/main/java/com/stepstone/stepper/adapter/AbstractFragmentStepAdapter.java

+        return createStep(position);
+    }
+
+    public abstract T createStep(int position);


This is no longer needed here since it's in StepAdapter?

zawadz88 · 2016-12-19T13:02:29Z

material-stepper/src/main/java/com/stepstone/stepper/type/TabsStepperType.java

        final int stepCount = stepAdapter.getCount();
        for (int i = 0; i < stepCount; i++) {
-            final Step step = stepAdapter.getItem(i);
+            final Step step = (Step) stepAdapter.createStep(i);


Due to the fact that StepAdapter no longer gets a this is needed, however this feels kind of nasty. I'm wondering if there's a better way of handling this. @fabiozo @leonardo2204 any ideas?

I'm not sure what you mean here, do you mean the casting ?

The object returned from stepAdapter.createStep(i) is casted to Step. This seems kind of weird since I would assume the createStep method to actually return a Step instance. I'm not sure if I explained this well enough :P

Now I get it, you're right. I was thinking about making public interface StepAdapter<T extends Step>, but then all the Frags example would break.
Do you have any ideas ? Gonna think about it for a time!

I've given some thought about it, maybe throwing an error on getItem because of invalid cast ?

I've added a new proposition: leonardo2204#1 please give me your thoughts ;)

zawadz88 · 2016-12-19T13:03:59Z

sample/src/main/res/layout/activity_main.xml

        android:paddingTop="@dimen/activity_vertical_margin"
        tools:context="com.stepstone.stepper.sample.MainActivity">

+        <Button


Could you move the button somewhere near the bottom, please? The idea is to have most common examples at the top and I think that most people still rely on Fragments

zawadz88 · 2016-12-19T13:07:38Z

sample/src/main/java/com/stepstone/stepper/sample/DifferentNextButtonStepperActivity.java

    }

-    private static class MyStepperAdapter extends AbstractStepAdapter {
+    private static class MyStepperAdapterFragment extends AbstractFragmentStepAdapter {


Probably this should be named MyStepperFragmentAdapter instead of MyStepperAdapterFragment?

leonardo2204 · 2016-12-19T13:56:24Z

As of the docs, once you think the PR is good to go, I'll add :)

- removed most of the casting from adapters

- refactored step adapters in the sample app

refactorings for stepstone-tech#37

leonardo2204 · 2017-01-10T16:12:50Z

@zawadz88 Done :)
Waiting for review and merge !

Thanks

zawadz88 · 2017-01-11T07:36:20Z

@leonardo2204 thanks for the changes!
This will be published to JCenter in version 2.0.0 as there are some breaking API changes.
I'm planning to release it by the end of January as there are some other improvements I want to add + update the documentation.

leonardo2204 · 2017-01-11T12:53:34Z

Sure, really appreciate all the help and attention !
Do you want me to update the docs also ?

leonardo2204 added 2 commits December 18, 2016 04:04

Initial commit

89442f9

Fixed manifest

891f40f

leonardo2204 changed the title ~~Further abstraction~~ [WIP]Further abstraction Dec 18, 2016

Final example

bfb0c70

leonardo2204 changed the title ~~[WIP]Further abstraction~~ Further abstraction Dec 18, 2016

zawadz88 suggested changes Dec 19, 2016

View reviewed changes

Added docs and changed a couple of stuff

401f6fb

Piotr Zawadzki and others added 5 commits December 19, 2016 22:35

- moved common methods to AbstractStepAdapter

c81afa3

- removed most of the casting from adapters

- removed getStepView methods

ca3c07a

cleared destroyed views in AbstractStepAdapter

f6d0f23

- extracted logic from AbstractStepAdapter to the sample adapter

14e6cd5

- refactored step adapters in the sample app

Merge pull request #1 from zawadz88/master

d21e6d6

refactorings for stepstone-tech#37

leonardo2204 mentioned this pull request Jan 11, 2017

Error back #43

Closed

zawadz88 approved these changes Jan 11, 2017

View reviewed changes

zawadz88 merged commit b2664f5 into stepstone-tech:master Jan 11, 2017

leonardo2204 mentioned this pull request Jan 11, 2017

Error back #44

Merged

Further abstraction #37

Further abstraction #37

Uh oh!

Conversation

leonardo2204 commented Dec 18, 2016

Uh oh!

leonardo2204 commented Dec 18, 2016

Uh oh!

zawadz88 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardo2204 commented Dec 19, 2016

Uh oh!

leonardo2204 commented Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zawadz88 commented Jan 11, 2017

Uh oh!

leonardo2204 commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

leonardo2204 commented Jan 10, 2017 •

edited

Loading

leonardo2204 commented Jan 11, 2017 •

edited

Loading