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

Grid rows ios fix #17199

Closed
wants to merge 3 commits into from
Closed

Conversation

john-hollander
Copy link
Contributor

Description of Change

Adjusted the DetermineMinimumStarSizesInSpan function to distribute the space needed relative to the target sizes associated with the star values using the same method as the ExpandStars method.

In the Measure method, the height constraint doesn't take the safe area into consideration for iOS, and this results in the Size value of the definitions to be higher than what is actually available in the safe area for each row. Then when the DetermineMinimumStarSizesInSpan was called with dimensions that were consistent with the safe area, it would apportion the space needed among the star rows without regard for how much of the Minimum size already existed for a row.

In the test code, this occurs when splitting the space needed into the second and third rows. The first row wasn't affected, so its height was the optimal, but the second row already had an optimal value and the third was empty. The old method made it such that the rows were 1.0, 1.5, 0.5 optimal height respectively, and then the second row was truncated to the optimal value that it would be without the safe area taken into account (so, slightly larger than optimal). The new method splits the space needed with the star sizes taken into account, and results in the rows being 1.0, 1.0, 1.0 optimal height respectively, consistent with the safe area.

As a result, the values sent into the ExpandStars method no longer include sizes that are inconsistent with the safe area, and now produce the expected results.

Issues Fixed

Fixes #17125

Clean up the full expansion check by removing the maxCurrentSize calculation (which is no longer used), and using a boolean instead of minimum star size so that we can exit the check early if the fit test fails.
Uses a distribution method similar to that used previously in the ExpandStars method in the DetermineMinimumStarSizesInSpan method to better distribute the space needed among the rows/columns.
Refactored star distribution code to allow both methods to use common code.
@john-hollander john-hollander requested a review from a team as a code owner September 4, 2023 21:49
@ghost ghost added the community ✨ Community Contribution label Sep 4, 2023
@ghost
Copy link

ghost commented Sep 4, 2023

Hey there @john-hollander! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@jfversluis jfversluis requested a review from hartez September 5, 2023 13:40
@jfversluis
Copy link
Member

/azp run MAUI-DeviceTests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hartez hartez removed the request for review from jsuarezruiz September 5, 2023 15:04
@samhouts samhouts added this to the .NET 8 GA milestone Sep 5, 2023
@Eilon Eilon added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Sep 5, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Sep 19, 2023
@baaaaif
Copy link

baaaaif commented Sep 26, 2023

I'm wondering why this fix isn't getting attention from the Maui team. My respect to @john-hollander for his work. Had to replace the GridLayoutManager code with his to address existing grid issues.
Why isn't there a review of this? Or at least a comment? If stale is already set...

@PureWeen PureWeen modified the milestones: .NET 8 GA, .NET 8 SR1 Sep 28, 2023
rmarinho added a commit that referenced this pull request Oct 16, 2023
…17880)

### Description of Change

For constrained measurement, when definition sizes are determined during
the measure pass the star sizes are limited by the number of stars which
can fit into the constraint. During arrange, if the actual arrange size
is different, then the size of a star definition needs to be limited by
the number of stars which can fit into the arranged size.

Proposing this as an alternative to #17199 - this is a less extensive
change to achieve the same effect.

### Issues Fixed

Fixes #17125
@rmarinho
Copy link
Member

Hey @john-hollander thank you for your contribution, after looking more a the issue @hartez has found a less intrusive change to the codebase here #17880 . So we are closing this one for now, but if we still have issues we can review this changes again.

@rmarinho rmarinho closed this Oct 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter community ✨ Community Contribution platform/iOS 🍎 stale Indicates a stale issue/pr and will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[regression/8.0.0-rc.1.9171] Grid rows aren't same height on iOS
7 participants