FoldableLayout icon indicating copy to clipboard operation
FoldableLayout copied to clipboard

Horizontal orientation

Open ghost opened this issue 9 years ago • 3 comments
trafficstars

Hey, I think that the previous issue should be fixed.

ghost avatar Mar 18 '16 09:03 ghost

Thank you for new feature, it is now possible to automatically merge your pull request. But I see several issues that prevents me from merging it right now:

  1. Code does not compile. Missing fl_orientation attribute, missing OrientationRecyclerView class and more.
    Not sure if i.e. OrientationRecyclerView class is really needed though, why don't use regular RecyclerView and set it up in activity?
  2. Code has lots of Checkstyle issues (run gradlew :library:checkstyle / gradlew :sample:checkstyle).
  3. New changes override my changes, i.e. in sample/build.gradle, UnfoldableDetailsActivity and other places.
  4. There are quite a lot of copy-paste blocks placed within if (orientation == VERTICAL) {...} else {...} statements. It would be better to generalize variables and computations instead of if statements with copy-pasted code.
  5. I would prefer to use already existing system orientation attribute, instead of custom fl_orientation. This will allow to keep library in .jar, instead of .aar archive.

As a side note, it would be really great if we could add more flexibility to this feature by using gravity instead of orientation. For example Gravity.TOP will mean regular direction, Gravity.BOTTOM will mean vertical unfolding animation but in opposite direction, Gravity.LEFT and Gravity.RIGHT (Gravity.START / Gravity.END) will mean horizontal animations. It should be quite straightforward to switch from orientation to gravity, but of course I don't insist that you should do it, once I merged your pull request I can make necessary changes myself.

alexvasilkov avatar Mar 20 '16 07:03 alexvasilkov

BTW, you should not need to close this pull request and open new one, just add new commits to this PR.

alexvasilkov avatar Mar 20 '16 07:03 alexvasilkov

Hi, thanks for the notes. Here's my reasoning.

  1. I used OrientationRecyclerView because you have to set LinearLayoutManager with appropriate orientation argument. It could be passed in the onCreate() method but it would be basically the same thing. I just used simplified version of my RecyclerView class from another project. If you want to make demo simpler I don't mind changing it.
  2. Yes, you're right. I made some copy-paste mistreating and used different notation. Fixed now.
  3. Right, I don't know why I added older versions of dependencies. sample/build.gralde should be alright now. As to overriding other changes could you please explain more? Does it undo yours? I don't quite understand.
  4. I added maybe one more tweak but I don't really know how much more it could be generalized. Different orientation conditions require either calling different methods or have nested conditions, and calculating variables beforehand would be inefficient. I might be completely wrong on this one but I just can't see it.
  5. Yes, of course. I changed it to default orientation.

As to gravity attribute, yes, it would be good extension but I would like to settle mentioned issues first. :)

I've just noticed that my push request didn't upload new classes and layout files. I'll check what is going on there.

ghost avatar Mar 20 '16 12:03 ghost