airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.5k stars 733 forks source link

Update sample on 2.0 branch to show new features #163

Closed elihart closed 7 years ago

elihart commented 7 years ago

To get ready for the 2.0 release we need to update the sample project to show how to use the new features, as well as take the opportunity for a few other improvements.

We should show:

  1. The new EpoxyController. I've already updated the sample to use this instead of the adapter.
  2. Building a view programmatically. I've created a Carousel view and CarouselModel that does this.
  3. Showing how to use a nested carousel. I've already added the Carousel view/model and have a basic usage in the sample. This should be improved though.
  4. Showing shared recycler view pools between all the carousels and main recyclerview.
  5. Demonstrate saving state. Save the state of all the data and restore from the activity.
  6. Demonstrate the new onModelClick listener. Possibly add a click listener to the color square that toggles its color.
  7. Demonstrate the onBind method. Not sure what the implementation would do.
  8. Demonstrate the EpoxyModelGroup to show how to group models.

To tie all this together I'm thinking that the current buttons (add/clear/shuffle/change) should add a horizontal carousel instead of a single color block (and maybe get rid of the change button), and then we can clear or shuffle the carousels.

The carousel group can be a EpoxyModelGroup with a horizontal linear layout. It can have buttons on the left side stacked vertically (add/clear/shuffle/change) and a Carousel taking up the rest of the horizontal space. The buttons would add/remove a color block into the carousel.

We currently store a list of ColorData to maintain state. This would be changed to List<List> to represent the nested carousels. The main activity would save and restore this state.

Besides that it would be nice to polish the UI with nicer colors, layout, and design. I'm not great at coming up with nice designs so I'm open to anybody willing to improve it if they have good ideas.

elihart commented 7 years ago

@geralt-encore are you still interested in working on this? I'd really appreciate it!

geralt-encore commented 7 years ago

Of course! Not sure how much I will able to do, but I'll definitely will try to help you as much as I can with this.

elihart commented 7 years ago

@geralt-encore thanks! Let me know if you have questions and what you think you might be able to do. Whatever you can't get to I'll probably work on in a week or so

elihart commented 7 years ago

@geralt-encore Have you had any time to work on this? I'm finishing up documentation and other changes and will start on this soon if you haven't

geralt-encore commented 7 years ago

Sorry, it was a tough week 😔 I might have some time on the weekend but can't really promise.

elihart commented 7 years ago

Ah, I know how that goes. Well, no pressure. I'll polish up documentation and some other changes over the next few days, and if I don't hear from you by early next week I'll start work on it

elihart commented 7 years ago

good luck with your other stuff!

geralt-encore commented 7 years ago

So I finally had some time and have started to work on this. But I stuck with EpoxyModelGroup and I am not sure if these are real limitations or I just didn't get something. First, EpoxyModelGroup doesn't support nested layouts, so it is not possible to use just a horizontal LinearLayout since it requires nested vertical LinearLayout for buttons. Probably, it should be possible to do with RelativeLayout or ConstraitLayout (I didn't manage to make it work with RelativeLayout, but I am not really that good with UI stuff so I might mess up). Anyway, it looks like a quite heavy limitation to me. Second, looks like there is no good way of dealing with a dynamic number of models (like in the case of buttons we need to show different amount of buttons based on data) in EpoxyModelGroup since the implementation is based on the order of ViewStubs. There is a workaround with providing a different layout for different cases, but it can get out of hands quickly if you have big enough number of variations.

elihart commented 7 years ago

@geralt-encore Thanks for starting work on it, and letting me know about the nested view group problem. I had never actually tried that... Trying it now the problem seems to be that I wasn't doing a recursive check for view stubs in

private int getNextViewStubPosition(ViewGroup groupView) {
      int childCount = groupView.getChildCount();
      for (int i = 0; i < childCount; i++) {
        View child = groupView.getChildAt(i);

        if (child instanceof ViewStub) {
          return i;
        }
      }

      throw new IllegalStateException(
          "Your layout should provide a ViewStub for each model to be inflated into.");
    }

to handle nested views. Should be an easy fix! I don't think there is any reason why we shouldn't be able to support the nested linear layouts.

As for the second point, the way I envisioned handling that is hiding the models that you don't want shown, and those views will that be ignored. I think it would work well for this case.

I'm making some changes to the library to support this and will push them soon. thanks for pointing it out!

elihart commented 7 years ago

I fixed the nested view stub issue in https://github.com/airbnb/epoxy/pull/176 as well as some other things. I also made the change to use carousels.

image

So remaining tasks are:

I also think that the buttons on the side of the carousel don't look good since they take up too much space. I think using icons arranged in a 2x2 grid could look better

I also realized that the carousel scroll position won't save correctly because of https://github.com/airbnb/epoxy/issues/175. That would be nice to fix but isn't a priority.

@geralt-encore Sorry to set you up for failure on the carousel work :( but thanks for letting me know about the issues. These remaining tasks should be more straightforward if you are still interested in working on this

geralt-encore commented 7 years ago

@elihart no problem! It's good that we discovered that nested group problem early. I'll see what I can do with the remaining issues.

geralt-encore commented 7 years ago

@elihart I've added onModelClickListener to ColorModel. I didn't get what you mean by saving state. Isn't it handled automatically or am I missing something? Regarding buttons in carousels - it is also possible to increase the size of color squares. Then there will be place just for 4 squares near the buttons and they will have roughly the same height. So it should look better.

elihart commented 7 years ago

@geralt-encore Thanks for the click listener work!

So Epoxy will save the state of views as you scroll around the recyclerview, but if we want to save state across the activity lifecycle (eg rotation) we also need to call onSaveInstanceState on the controller, and restore the state when we create a new instance of the controller by passing the Bundle as a param to the constructor and calling onRestoreInstanceState.

However, that only saves view state, and the carousel and color data will also need to be saved. I'm thinking we need to implement parcelable in those two data models and then save/restore them in the main activity.

elihart commented 7 years ago

Almost done! I don't think the onBind example is necessary. It's pretty discoverable and easy to use, and I can't think of a good reason to use it in the sample.

Last thing to do is just do any last UI polish and record of GIF example. If anyone likes UI polish and has ideas to do that, go for it!

elihart commented 7 years ago

Done :)