Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] CollectionView item spacing #4681

Closed
pauldipietro opened this issue Dec 8, 2018 · 16 comments
Closed

[Enhancement] CollectionView item spacing #4681

pauldipietro opened this issue Dec 8, 2018 · 16 comments

Comments

@pauldipietro
Copy link
Contributor

@pauldipietro pauldipietro commented Dec 8, 2018

Summary

The current CollectionView spec (#3172) lacks the ability to specify the amount of space between items inside of it. There are, of course, cases where a user might wish for specific spacing, and could be especially useful with the Grid layout type that CollectionView provides.

API Changes

ListItemsLayout:

  • public double ItemSpacing { get; set; }

GridItemsLayout:

  • public double VerticalItemSpacing { get; set; }
  • public double HorizontalItemSpacing { get; set; }

Intended Use Case

Provide adequate space between items inside of a CollectionView.

On iOS, these will map to GetMinimumLineSpacingForSection and GetMinimumInteritemSpacingForSection. On Android, these will add a subclass of RecyclerView.ItemDecoration which will provide the spacing between items.

@rasmuschristensen
Copy link

@rasmuschristensen rasmuschristensen commented Dec 10, 2018

Where would the ItemSpacing be added? Currently I can not see a way of doing this?

@krdmllr
Copy link
Contributor

@krdmllr krdmllr commented Jan 1, 2019

Does this also affect the edge spacing? As i describe here #3172 (comment) we need something in addition to the CollectionView margin to have the same spacing between the columns in a grid layout and the sides of the grid.

For example, having a thickness of 5 on all items would result in a spacing of 10 between the items and a spacing of 5 on the outside of the grid. In this case, something like an EdgeOffset of 5 would be required to keep the same spacing in- and outside of the grid.

@samhouts samhouts added this to the 4.0.0 milestone Jan 10, 2019
@hartez
Copy link
Member

@hartez hartez commented Jan 29, 2019

@pauldipietro I've added ItemSpacing to the CollectionView spec.

@hartez
Copy link
Member

@hartez hartez commented Jan 29, 2019

@krdmllr My first instinct is to say "yes, this will also effect edge spacing"; i.e., it would work like your example above with a Thickness of 5 -> 5 around the edges, 10 in between items. But I'll need to dig into the actual implementation on each platform to be totally sure; if we can reasonably limit this to inter-item spacing and leave the edges alone, then we might do that just so it's less mental effort for devs to figure out what effect margin/padding will have.

I suspect in either case we will need to address the issues you mentioned in your comment on #3172; I just don't know yet how we'd want to go about handling an EdgeOffset (or whatever we end up calling it).

@rmarinho
Copy link
Member

@rmarinho rmarinho commented Feb 3, 2019

So in #4996 I talked in adding also Padding so we can apply "insets" to the Viewport. Take note insets on CarouselView are also handle by the renderers so you can be able to select the 1st item if multiple items are on the screen and we need to push the 1st item to the center

@samhouts samhouts added this to Under consideration in Enhancements Feb 5, 2019
@samhouts samhouts moved this from Under consideration to Needs Specification in Enhancements Feb 6, 2019
@samhouts samhouts added this to To do in v4.0.0 Mar 13, 2019
@adrianknight89
Copy link
Contributor

@adrianknight89 adrianknight89 commented Mar 28, 2019

@hartez In my opinion, ItemSpacing should be a separate property that translates directly to InterimSpacing/LineSpacing (vertical/horizontal) whereas section insets should map to Padding.

ItemSpacing should not do the job of Padding. If there is a calculation issue (e.g. 10 between items, 5 on the edges), then the dev should also set Padding to correct this.

OR make Margin work for items so that there is no need for ItemSpacing and as I argued before, let people use Padding on the CollectionView to even out the extra space.

@davidortinau
Copy link
Contributor

@davidortinau davidortinau commented Mar 30, 2019

What's the status of this, still needs specification?

From a DX standpoint, I first tried to add a Margin to my Frame within the ItemTemplate, and it did nothing. I then went looking to any option to set Spacing on either the GridItemsLayout or CollectionView.

I then solved this by adding a ContentView and setting Padding to achieve the spacing. O @jassmith forgive me!

And finally I came here.

Screenshot 2019-03-30 12 56 56

@krdmllr
Copy link
Contributor

@krdmllr krdmllr commented Mar 30, 2019

@davidortinau this still has the problem that the spacing between the items (10+10) is not the spacing the outer items have to the window (10). But i think your point is that your way the shadow of the frame still works?

edit: I would vote for an item spacing and padding property on the collection view since stack layout and grid both have a item spacing property aswell (RowSpacing/ColumnSpacing/Spacing)

@davidortinau
Copy link
Contributor

@davidortinau davidortinau commented Mar 30, 2019

Perhaps I'm overselling it by saying I "solved" anything other than getting space between my items. You're correct @krdmllr that this produces different spacing between than outside. Margin would have a similar outcome.

@hartez
Copy link
Member

@hartez hartez commented Jun 3, 2019

I whipped up a possible implementation of this, and I'm looking for feedback: https://github.com/xamarin/Xamarin.Forms/tree/cv-itemspacing

Rather than using Thickness, I've added bindable properties to the layout classes:

double ListItemsLayout.ItemSpacing
double GridItemsLayout.VerticalItemSpacing
double GridItemsLayout.HorizontalItemSpacing

On iOS, this maps to GetMinimumLineSpacingForSection and GetMinimumInteritemSpacingForSection. On Android, these add a subclass of RecyclerView.ItemDecoration called SpacingItemDecoration.

You can play around with them in the ControlGallery on that branch; just navigate to CollectionView Gallery -> ItemsSpacingGallery.

@hartez
Copy link
Member

@hartez hartez commented Jun 3, 2019

If folks don't immediately hate it or find glaring omissions, I'll update the spec and PR it.

@hartez hartez mentioned this issue Jun 10, 2019
2 of 3 tasks complete
@samhouts samhouts added this to In Progress in v4.1.0 Jun 10, 2019
@samhouts samhouts moved this from In progress to Ready for Review (Issues) in Sprint 154 Jun 10, 2019
@samhouts samhouts removed this from Needs Specification in Enhancements Jun 10, 2019
rmarinho added a commit that referenced this issue Jun 13, 2019
* First pass at item spacing for CollectionView ListItemsLayout on Android

* Handle GridItemsLayout spacing on Android

* Change spacings from int -> double
Implement spacing on iOS

* Remove old TODO comment
@samhouts samhouts moved this from In Progress to Done in v4.1.0 Jun 13, 2019
@samhouts samhouts moved this from Ready for Review (Issues) to Done in Sprint 154 Jun 13, 2019
@samhouts
Copy link
Member

@samhouts samhouts commented Jun 18, 2019

closed by #6478

@samhouts samhouts closed this Jun 18, 2019
v4.0.0 automation moved this from To do to Done Jun 18, 2019
@LeoJHarris
Copy link

@LeoJHarris LeoJHarris commented Jun 26, 2019

Is this available in a release yet? I was looking for the property on the CollectionView but was unable to find it? Perhaps it hasnt been rolled out yet?

Im using XF 4.1.0.496342-pre2 and it looked like this was in the 4.1.0 milestone.

@kramer-e
Copy link

@kramer-e kramer-e commented Jul 31, 2019

It works in XF 4.2.0.618605-pre2.

<CollectionView.ItemsLayout>
    <ListItemsLayout Orientation="Vertical"
                     ItemSpacing="10" />
</CollectionView.ItemsLayout>
@NirmalSubedi17
Copy link

@NirmalSubedi17 NirmalSubedi17 commented Aug 20, 2019

We have issue on HorizontalItemSpacing. Even when we set "0" some default spacing is there. No issue with VerticalItemSpacing though.
See the yellow lines between the columns of the Collection view:

Screen Shot 2019-08-20 at 5 34 46 PM

This is the code to reproduce:

<StackLayout Margin="10,50,10,5" Padding="0" Spacing="0" BackgroundColor="Yellow" HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand">
        <CollectionView ItemsSource="{Binding GameCells}" Margin="0" HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand">
            <CollectionView.ItemsLayout>
                <GridItemsLayout Orientation="Vertical" Span="9" HorizontalItemSpacing="0" VerticalItemSpacing="0"/>
            </CollectionView.ItemsLayout>
            <CollectionView.ItemTemplate>
                <DataTemplate>
                    <ContentView BackgroundColor="Green" Padding="1" Margin="0" HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand">
                        <ContentView BackgroundColor="White" Padding="10" Margin="0" HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand">
                            <Label Text="{Binding DisplayText}" FontSize="Large" FontAttributes="Bold" HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand"
                                   HorizontalTextAlignment="Center" VerticalTextAlignment="Center"/>
                        </ContentView>
                    </ContentView>
                </DataTemplate>
            </CollectionView.ItemTemplate>    
        </CollectionView>
    </StackLayout>
@hartez
Copy link
Member

@hartez hartez commented Aug 20, 2019

We have issue on HorizontalItemSpacing. Even when we set "0" some default spacing is there. No issue with VerticalItemSpacing though.

@NirmalSubedi17 Please open a new issue at https://github.com/xamarin/Xamarin.Forms/issues/new/choose and we will take a look. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Sprint 154
  
Done
v4.0.0
  
Done
v4.1.0
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

None yet