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

fix: Guild Member List & Guild Search Lists are now fully clickable + Grid fixes #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HiFlii
Copy link

@HiFlii HiFlii commented Feb 9, 2025

This introduces two fixes: one for the Guild Menu and one for Grid in general

Copying my commit messages here:

Grid

Previously the grid would give an extra line if the placeables
was evenly divisible by itemsPerLine.

Guild Menu

Previously, the guild member list menus had the rightmost and
bottommost column/row (respectively) not be clickable. This occured
because the contents (usually a Grid) was offset and the Box containing
them was not.

This has been resolved through and admitedly hacky solution where:

  1. A box is rendered as before and used to fetch the size of the contents (Grid)
  2. A box is rendered whose components are blank cells to clear out step (1)
  3. The final box is rendered using fillMaxSize() to ensure that the contents
    fit inside it.

This separates the size extraction and the content containing steps.

This ensures that:

  • The 'itemsPerPage' or 'itemsPerLine' and 'totalLines' variables
    are of the size of the original contents instead of their enclosing Box.

  • The enclosing box for the contents is of sufficient size such that all
    the elements are clickable.

Alternative solutions could include altering the spec for Scrollable and
Paginated to take in an optional Size override.

Previously the grid would give an extra line if the placeables
was evenly divisible by itemsPerLine.
Previously, the guild member list menus had the rightmost and
bottommost column/row (respectively) not be clickable. This occured
because the contents (usually a Grid) was offset and the Box containing
them was not.

This has been resolved through and admitedly hacky solution where:

1) A box is rendered as before and used to fetch the size of the contents (Grid)
2) A box is rendered whose components are blank cells to clear out step (1)
3) The final box is rendered using fillMaxSize() to ensure that the contents
fit inside it.

This separates the size extraction and the content containing steps.

This ensures that:

- The 'itemsPerPage' or 'itemsPerLine' and 'totalLines' variables
are of the size of the original contents instead of their enclosing Box.

- The enclosing box for the contents is of sufficient size such that all
the elements are clickable.

Alternative solutions could include altering the spec for Scrollable and
Paginated to take in an optional Size override.
@HiFlii HiFlii requested a review from 0ffz as a code owner February 9, 2025 21:19
@0ffz
Copy link
Member

0ffz commented Mar 3, 2025

I can cherry pick the width and height fixes into the 1.21.4 branch, however the double composition of content() isn't great and I'd like to avoid that. Compose on android has layouts that can measure their own size in a better way than what we have, I'm hoping to improve the behaviour with something similar in the future. I think this specific issue can be fixed on the guild implementation end for now though, will try to have a look when I'm done cleaning those up.

@HiFlii
Copy link
Author

HiFlii commented Mar 4, 2025

Sounds good. We can revisit once you finish the guild implementation cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants