Add pagingEnabled to the grid and a GMGridViewLayoutHorizontalPagedLtr #1

Closed
wants to merge 8 commits into
from

Projects

None yet

2 participants

@rantav
rantav commented Nov 16, 2011

Add pagingEnabled to the grid. It simply delegates the property back to the undelying scrollView
Implement a new layout strategy: GMGridViewLayoutHorizontalPagedLtr
And add a few geometry translators that'll translate points and rectangles from the underlying scrollview to the desired view

rantav added some commits Nov 16, 2011
@rantav rantav Add pagingEnabled to the grid. It simply delegates the property back …
…to the undelying scrollView
b128ad4
@rantav rantav Fix name of isPagingEnabled to pagingEnabled. Old java habits... d58ccf1
@rantav rantav Implement GMGridViewLayoutHorizontalPagedLtr. a layout for paged hori…
…zontal scroll where items in each page flow from top-left to bottom-right
abf5a73
@rantav rantav Add convertScrolledPoint and convertScrolledRect so users of GMGridVi…
…ew are able to get the position of a scrolled element
35b4bda
@rantav rantav Add 'build' to gitignore 53f7a0e
@rantav rantav Add support for better programatic scroll control.
Expose the contentOffset
Allow scrolling to a particular point (and not only to a particular item)
Add more page queries to the GMGridViewLayoutHorizontalPagedLtrStrategy
... it's all part of the same scheme...
92aa779
@rantav rantav Add GMGridViewDidScroll so that users know when GMScrollView is scrol…
…ling
a6d3057
@gmoledina
Owner

Looks good ! I'll check it tomorrow and merge it.

@rantav
rantav commented Nov 20, 2011

I added more stuff to this pull request (sorry, I couldn't find a way to create a completely new pull request):

Implement a new layout strategy: GMGridViewLayoutHorizontalPagedLtr

And add a few geometry translators that'll translate points and rectangles from the underlying scrollview to the desired view

@gmoledina
Owner

Thanks :)

I looked into this and it's breaking the way the logic is separated right now between the layoutStrategy classes and the main gridView class (for example, the layout isn't supposed to know about any spacing).
Also, it requires users to enable paging themselves and all the other animations when inserting/deleting items don't work for this layout.

I think the paging should be automatically enabled based on the layout being used (a method in the layout class ?) and either keep all the spacing logic in the layout classes or in the gridView.

I've started to think about it, let me know if you have any suggestions !

@rantav
rantav commented Nov 21, 2011

I agree with your dislike of coupling the layout strategy and the
requirement to use pagingEnabled and zero spacing (minEdgeInsets). I hated
myself for having to write those lines of documentation and would be happy
to remove them and 'just make it work'

I have a few suggestions to solve that:

  1. The grid view would set paging and spacing automatically to the correct
    values in the case that the paged-layout strategy is selected. This would
    'make things just work' in the general sense, however might leave users
    wondering what's going on and why are their spacing settings aren't being
    applied if they happen to set them explicitly. This could be mitigated in
    documentation but who reads them? Another option is assert() in code in
    case a user is trying to set the paging or spacing and use the paged-layout
    strategy at the same time. This solution is asymmetric so it calls for
    generalization, not the prettiest design.
  2. Add two methods to the definition of GMGridViewLayoutStrategy -
    pagingEnabled and minEdgeInsets and implement those in all the layout
    strategies. At the same time, remove those two from GMGridView. This would
    make things more explicit, but perhaps also a bit more 'bureaucratic'. They
    would both have good defaults and allow users to override them at their own
    peril. What do you think?

I'm happy to get your take on this.

On Mon, Nov 21, 2011 at 4:04 AM, Gulam Moledina <
reply@reply.github.com

wrote:

Thanks :)

I looked into this and it's breaking the way the logic is separated right
now between the layoutStrategy classes and the main gridView class (for
example, the layout isn't supposed to know about any spacing).
Also, it requires users to enable paging themselves and all the other
animations when inserting/deleting items don't work for this layout.

I think the paging should be automatically enabled based on the layout
being used (a method in the layout class ?) and either keep all the spacing
logic in the layout classes or in the gridView.

I've started to think about it, let me know if you have any suggestions !


Reply to this email directly or view it on GitHub:
#1 (comment)

/Ran
http://tavory.com

@gmoledina
Owner

Scrolling to a particular point will be weird when paging is enabled

OK, this commit has two different things even though in my mind they are related.
We have the two geometry methods:

  • (CGPoint)originForItemAtColumn:(NSInteger)column row:(NSInteger)row page:(NSInteger)page;
  • (CGPoint)originForPage:(NSUInteger)page;

They are useful to know the physical location on the screen a certain item in a page (I use it to 'fly' things away from the grid off to another UIView in an animation when the user touches them)

The second thing is

  • (void)setContentOffset:(CGPoint)offset animated:(BOOL)animated;

Well, I have a paging control that I use to scroll b/w pages. So I use a combination of originForPage and setContentOffset.
I see why this breaks encapsulation and I'm open for suggestions... the goal is to allow an external page control to slide switch pages in the grid.

As a matter of fact when I come to think of it, regarding setContentOffset and pagingEnabled - this is consistent with how UIScrollView behaves. Event in UIScrollView it's possible to programmatically scroll to any point in the scroll view even though the scroll view is paged. So although I agree with you that this is breaking encapsulation to some degree, at least we didn't break it first and we're consistent with apple model ;)

@gmoledina
Owner

I agree.
I'll add this on my next update when I'll add an API to allow having a pageControl (so knowing when the page changed).

@gmoledina
Owner

Thanks! I have implemented a solution close to what we discussed.

Instead of having the user control the pagingEnabled property of the scrollView, it is set automatically by the gridView depending on the layoutStrategy.

The edgeInsets (minEdgeInsets and centerGrid) logic is moved to the layout classes. The edgeInset is set on each page of the gridView (makes more sense); same applies to centerring the grid".

I just pushed the new code, let me know what you think.

@gmoledina gmoledina closed this Dec 12, 2011
@gmoledina
Owner

Is it better to have just one property that controls both horizontal and vertical scroll indicator ?
As we never have both of them visible at the same time anyway.

Owner

Alright! I've pulled your code on my local repo. It will be part of the next release.
Thanks!

@rantav
rantav commented Dec 12, 2011

cool, I hope to give it a spin the next coulpe of days (although
I'm preoccupied with app store submission right now, but I'd like to spend
some cycles on the grid as well).
You know what could totally make me go for the latest grid version? that's
right, you guessed right - iOS 4 support ;)

On Mon, Dec 12, 2011 at 2:56 AM, Gulam Moledina <
reply@reply.github.com

wrote:

Thanks! I have implemented a solution close to what we discussed.

Instead of having the user control the pagingEnabled property of the
scrollView, it is set automatically by the gridView depending on the
layoutStrategy.

The edgeInsets (minEdgeInsets and centerGrid) logic is moved to the layout
classes. The edgeInset is set on each page of the gridView (makes more
sense); same applies to centerring the grid".

I just pushed the new code, let me know what you think.


Reply to this email directly or view it on GitHub:
#1 (comment)

/Ran
http://tavory.com

@gmoledina
Owner

Support for iOS 4 is 95% implemented :)
Will do some more testing and profiling before I can push the code.

Good luck with your release!

@rantav
rantav commented Dec 13, 2011

Awesome :)
On Dec 13, 2011 2:37 AM, "Gulam Moledina" <
reply@reply.github.com>
wrote:

Support for iOS 4 is 95% implemented :)
Will do some more testing and profiling before I can push the code.

Good luck with your release!


Reply to this email directly or view it on GitHub:
#1 (comment)

@ralcazar ralcazar referenced this pull request May 18, 2012
Closed

Problem reusing cells #106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment