Skip to content

Migrate FloatingButton to use new ScreenFooter component internally#3948

Open
yotam-wix wants to merge 7 commits intomasterfrom
feat/floatingButton-migration
Open

Migrate FloatingButton to use new ScreenFooter component internally#3948
yotam-wix wants to merge 7 commits intomasterfrom
feat/floatingButton-migration

Conversation

@yotam-wix
Copy link
Collaborator

@yotam-wix yotam-wix commented Mar 12, 2026

Description

  • Rewrote FloatingButton component to a function component that renders ScreenFooter internally, while keeping the same external API and visual behavior.
  • All existing props work as before, mapped to their ScreenFooter equivalents.
  • Added hoisted prop (default: true) that gives FloatingButton built-in keyboard-awareness using screenFooter's logic.
  • Updated FloatingButtonScreen to use SafeAreaProvider

Changelog

floatingButton - changed to use new ScreenFooter component internally. deprecated original component.

Additional info

Ticket 5000

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

✅ 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.

@yotam-wix yotam-wix changed the title complete internal migration of floatingButton and its screen to use f… Migrate FloatingButton to use new ScreenFooter component internally Mar 12, 2026
@yotam-wix
Copy link
Collaborator Author

few things regarding the migration -

  1. I've split up the changes to few commits, the first one is the core logic change of the floatingButton component itself and the screen, then there are the tests and api.json file updates.
  2. When the footer is used indirectly via floatingButton, it's missing a safeAreaProvider. Perhaps we need to consider wrapping the useSafeAreaInsets inside the screenFooter itself in a try/catch block and let our users know they have to wrap it themselves.
  3. Currently, the floatingButton has a bottomMargin prop which controlled the content styling itself. the issue is - footer doesn't have a current support for that, so the component now ignores that prop. some users of us could notice a styling change, and to fix it we could introduce a new contentContainerStyle or bottomMargin prop to screenFooter. WDYT?

@lidord-wix
Copy link
Collaborator

  • When the footer is used indirectly via floatingButton, it's missing a safeAreaProvider. Perhaps we need to consider wrapping the useSafeAreaInsets inside the screenFooter itself in a try/catch block and let our users know they have to wrap it themselves.
  • Regarding the useSafeAreaInsets - it is already wrapped with a try/catch under the optional dependency file, but maybe we can add a warning in the floating button to let the users know that in case they have issues with the safe area they need to wrap it with the provider.. (we have a logService we can use)
  • Regarding the bottomMargin - yes, let's add a prop to screenFooter (contentContainerStyle sounds good) and pass the bottom margins there, to make sure the UI will not break for users :)

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.

I added some comments, overall looks good :)
Please make the changelog (in the PR description) more clear 🙏🏽
Also - I wasn't able to run locally, had some issues with the yarn install.. will try again tomorrow


return (
<ScreenFooter
visible={visible}
Copy link
Collaborator

Choose a reason for hiding this comment

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

visible is undefined by default in the floating button and true in the screen footer, means that in case the user didn't pass anything and expected it to be invisible, now it will be shown


const TestCase = (props) => {
return <FloatingButton {...props} testID={TEST_ID}/>;
return <FloatingButton hoisted={false} {...props} testID={TEST_ID}/>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the tests are for the hoisted={false} case, let's add some for the default one

});
});

describe('bottomMargin', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's bring back all the bottomMargin tests to make sure the users aren't break

Copy link
Collaborator

Choose a reason for hiding this comment

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

please format all files before merging the PR (cmd + shift + P -> Format Document) make sure you're doing it with Prettier ESlint (cmd + shift + P -> Format Document With)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants