Add support for rendering FloatingButton secondary button standalone #3862
Conversation
|
@adids1221 please merge master after the monorepo change 🙏 |
| expect(await buttonDriver.exists()).not.toBeTruthy(); | ||
|
|
||
| renderTree.rerender(<TestCase visible/>); | ||
| renderTree.rerender(<TestCase visible button={button}/>); |
There was a problem hiding this comment.
Why did you need to add this to the test? It's a problem if the default behavior has changed.
There was a problem hiding this comment.
I made a breaking change here, and I believe it's necessary.
Let me explain:
Previous behavior: When rendering FloatingButton with only visible: true (without button or secondaryButton props), it rendered an empty button wrapper with no title/label, which was redundant and not useful.
New behavior: Users must now explicitly provide either button or secondaryButton props to render buttons. If neither is provided, the component returns null (no rendering).
Why this change:
- Removes redundant rendering of empty button wrappers
- Makes the API explicit: provide a button prop to render a button
- Improves clarity and prevents confusion from empty buttons
This breaking change is documented in the PR description under "Breaking Changes".
We can discuss if you'd like to adjust the approach.
There was a problem hiding this comment.
@adids1221
Let's talk about this - not sure it qualifies as a breaking change, but if it does we cannot merge it
There was a problem hiding this comment.
Let's remove the breaking change part from the Changelog (it's also generally best to have the Changelog in a single line unless it's actually two features) and add a
| marginBottom: Spacings.s7, | ||
| marginLeft: 20 | ||
| }, | ||
| fullWidthSecondary: { |
There was a problem hiding this comment.
now you can see the usage in renderSecondaryButton
packages/react-native-ui-lib/src/components/floatingButton/index.tsx
Outdated
Show resolved
Hide resolved
- Add support for rendering FloatingButton with only secondary button - Secondary-only mode automatically uses horizontal layout with outline styling - Add isSecondaryOnly and shouldSecondaryButtonFlex getters - Fix fullWidth margin consistency for secondary-only button - Refactor rendering logic to use positive checks for better readability - Fix fullWidth support in horizontal layout with only primary button Breaking: FloatingButton now requires button prop to render primary button (previously rendered empty wrapper with only visible: true)
| const buttonStyle = shouldUseHorizontalStyle | ||
| ? [ | ||
| styles.shadow, | ||
| fullWidth && this.isSecondaryOnly ? styles.fullWidthSecondaryMargin : styles.secondaryMargin, |
There was a problem hiding this comment.
Isn't secondaryMargin and fullWidthSecondaryMargin the same?
There was a problem hiding this comment.
you are right, fixed.
| marginBottom: Spacings.s7, | ||
| marginLeft: 20 | ||
| }, | ||
| fullWidthSecondary: { |
| return secondaryButton && !this.isHorizontalLayout; | ||
| } | ||
|
|
||
| get shouldSecondaryButtonFlex() { |
There was a problem hiding this comment.
This appears only once and the name is not too good, maybe we can remove the function and create a const below?
Not sure if there are other functions we can do that with.
| return ( | ||
| <View | ||
| row={this.isHorizontalLayout} | ||
| center={!!shouldCenter} |
There was a problem hiding this comment.
Not sure you need the !!
| const {withoutAnimation, visible, fullWidth, testID} = this.props; | ||
| // NOTE: keep this.firstLoad as true as long as the visibility changed to true | ||
| const {withoutAnimation, visible, fullWidth, testID, button, secondaryButton} = this.props; | ||
|
|
There was a problem hiding this comment.
Why did you remove the two comments?
There was a problem hiding this comment.
not sure, revert this change
| const bottom = this.isSecondaryVertical ? Spacings.s4 : bottomMargin || Spacings.s8; | ||
| const left = this.isSecondaryHorizontal || fullWidth ? Spacings.s4 : undefined; | ||
| const right = this.isSecondaryHorizontal ? 20 : fullWidth ? Spacings.s4 : undefined; |
There was a problem hiding this comment.
I know this is not your code, WDYT about making it marginBottom etc?
| const left = this.isSecondaryHorizontal || fullWidth ? Spacings.s4 : undefined; | ||
| const right = this.isSecondaryHorizontal ? 20 : fullWidth ? Spacings.s4 : undefined; | ||
| const shadowStyle = button && !button.outline && !button.link ? styles.shadow : undefined; | ||
| const marginStyle = {marginTop: 16, marginBottom: bottom, marginLeft: left, marginRight: right}; |
There was a problem hiding this comment.
We can also change 20 and 16 to Spacings.s5 and Spacings.s4, not a must
|
|
||
| if (secondaryButton) { | ||
| const bgColor = secondaryButton.backgroundColor || Colors.$backgroundDefault; | ||
| const shouldUseHorizontalStyle = this.isHorizontalLayout; |
There was a problem hiding this comment.
Why not use this.isHorizontalLayout?
There was a problem hiding this comment.
There are multiple usage here of shouldUseHorizontalStyle I think it's better for readability
Description
Adds support for rendering FloatingButton component with only the secondary button. When
secondaryButtonis provided withoutbuttonprop, the component renders the secondary button with horizontal layout styling and outline stroke.Breaking Changes:
buttonprop to render primary button (previously rendered empty wrapper with onlyvisible: true)Logic Changes:
Changelog
FloatingButton - Added support for rendering secondary button standalone.
Breaking:
buttonprop is now required to render primary button.Additional info
Support channel slack thread