Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[iOS] Fix MeasureFirstItem for CollectionView #7622

Merged
merged 9 commits into from
Sep 27, 2019

Conversation

adrianknight89
Copy link
Contributor

Description of Change

While looking at #5455, I noticed MeasureFirstItem does not work either. I provided a button to switch between MeasureFirstItem and MeasureAllItems. You could use this test as well as the gallery test in #5455 when figuring out a fix for MeasureAllItems.

MeasureAllItems:
To me, MeasureAllItems seems to be working in the test provided here. I assumed that each column would be as wide as the largest item in that column, and the other item would be centered. This is the case in this test. However, the first item is left aligned when the page is initially shown. If you scroll right and left, you'll see that the first item will be centered eventually. Something seems odd about the 4th (yellow) item. I was expecting it to be a bit wider since there is a lot of empty space to leverage. My head hurts too much right now to think about image AspectFit vs CollectionView item sizing, so I wanted to document it here for now. :)

Issues Resolved

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

ConstrainedDimension = constraint.Height;
Layout(constraint);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartez Previously, there was a Layout() call inside ConstrainTo(), but PreferredLayoutAttributesFittingAttributes does layout too. Any reason for that? Removing the call here made cell sizing better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I don't know why there was a Layout call there. Seems to work fine without it.

// Measure this cell (including the Forms element)
var size = Measure();
// Measure this cell (including the Forms element) if there is no constrained size
var size = ConstrainedSize == default(CGSize) ? Measure() : ConstrainedSize;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason MeasureFirstItem was broken is we were measuring here and assigning a new size to the cell. Since MeasureFirstItem constrains us to the first item, I added ConstrainedSize which is passed down from PrepareCellForLayout() in ItemsViewLayout as ItemSize.


// Reset constrained size in case ItemSizingStrategy changes
// and we want to measure each item
ConstrainedSize = default(CGSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could test this functionality in CollectionView Galleries -> Item Size Galleries -> ItemSizing Strategy.

  • Changing ItemSizingStrategy to measure all should measure each cell in PreferredLayoutAttributesFittingAttributes.
  • Changing ItemSizingStrategy to measure all and then back to measure first should not break measure first.

@adrianknight89
Copy link
Contributor Author

@rmarinho I'll drop the private keyword from here too once the tests are done.

@rmarinho
Copy link
Member

rmarinho commented Sep 26, 2019

The previous commit passed IOS uitests @hartez

Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

ConstrainedDimension = constraint.Height;
Layout(constraint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I don't know why there was a Layout call there. Seems to work fine without it.

@samhouts samhouts merged commit c09c373 into xamarin:4.3.0 Sep 27, 2019
@samhouts samhouts added this to the 4.3.0 milestone Oct 1, 2019
felipebaltazar pushed a commit to felipebaltazar/Xamarin.Forms that referenced this pull request Oct 16, 2019
* fix MeasureFirstItem

* added test

* changed file name

* fix path

* added text

* remove 5455

* drop private
fixes xamarin#7621
felipebaltazar pushed a commit to felipebaltazar/Xamarin.Forms that referenced this pull request Oct 16, 2019
* fix MeasureFirstItem

* added test

* changed file name

* fix path

* added text

* remove 5455

* drop private
fixes xamarin#7621
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants