Skip to content

ScreenFooter - fix overflow in Fit mode, fix stretching issues#3954

Open
yotam-wix wants to merge 2 commits intomasterfrom
fix/screenFooter-stretch-issues
Open

ScreenFooter - fix overflow in Fit mode, fix stretching issues#3954
yotam-wix wants to merge 2 commits intomasterfrom
fix/screenFooter-stretch-issues

Conversation

@yotam-wix
Copy link
Collaborator

Description

  • Fixed layout bugs in horizontal STRETCH, FIXED, and FIT modes caused by the previous row + justifyContent: center approach for centering items. That approach centered items but prevented buttons from stretching,

  • Replaced with column-direction wrappers (where default alignItems: 'stretch' handles button stretching) and cloneElement with the center modifier to inject centering into children's internal layout. This is necessary because alignItems: 'stretch' and alignItems: 'center' can't both be applied to the same container - the wrapper handles stretch, and the clone handles centering.

  • Restored flexShrink wrapping for horizontal FIT mode, which was lost during the earlier refactor.

Changelog

ScreenFooter - overflow and stretching bug fixes.

Additional info

N/A

@github-actions
Copy link
Contributor

✅ PR Description Validation Passed

All required sections are properly filled out:

  • Description
  • Changelog
  • Additional info

Your PR is good for review! 🚀


This validation ensures all sections from the PR template are properly filled.

Copy link
Collaborator

@lidord-wix lidord-wix left a comment

Choose a reason for hiding this comment

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

Added a comment

<View key={index} style={fixedStyle}>
{child}
{isHorizontal && React.isValidElement(child)
? React.cloneElement(child as React.ReactElement<any>, {center: true})
Copy link
Collaborator

Choose a reason for hiding this comment

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

We previously talked about it that we don't want to clone the user's element and add it some props
It's reasonable that their component won't even have the center prop..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I know. But we do need to handle the combination of 'alignItems: stretch' and centerization, which I couldn't find a proper solution for without cloning the children and injecting the centering to them somehow.

All previous solutions were messing up one of the two, and I can't find an alternative to cloneElement so far :/

Also, the user can always override the 'center' prop for one of his items manually which I felt made sense for a 'generic' component like the footer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally this bug is only related to components that do not already handle centering their content - such as Image for example. So technically we can move the responsibility to the users - by letting them know they need to center manually the items they're passing in that case.

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