-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
chore: remove Text component wrappers from BottomSheetHeader children #22173
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
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| <Text variant={TextVariant.HeadingMD}> | ||
| {strings('app_settings.select_rpc_url')}{' '} | ||
| </Text> | ||
| {strings('app_settings.select_rpc_url')}{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Broken styling for mixed header children in BottomSheet
The BottomSheetHeader now receives mixed children (a string, a space string, and a Cell component). The HeaderBase component only applies text styling when typeof children === 'string', which will be false for this array of mixed children. This means the string portion will render without proper styling. The previous Text wrapper ensured proper styling for all content. This should either keep the Text wrapper for the string portion or restructure the children to be a single string passed to BottomSheetHeader with the Cell component moved outside.
e19ecd6 to
2c87ef1
Compare
Pull request was converted to draft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the BottomSheetHeader component usage throughout the codebase by removing redundant text wrapping. The BottomSheetHeader component already handles text rendering internally via HeaderBase, which automatically applies the appropriate text variant based on the header variant (Compact or Display).
Key Changes:
- Removed explicit
<Text variant={TextVariant.HeadingMD}>wrappers fromBottomSheetHeaderchildren across multiple components - Removed unused
TextandTextVariantimports where they are no longer needed - Updated test snapshots to reflect the new text styling (HeadingSM for Compact variant instead of HeadingMD)
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| app/components/Views/TooltipModal/index.tsx | Simplified BottomSheetHeader by removing Text wrapper |
| app/components/Views/Settings/NetworksSettings/NetworkSettings/index.js | Removed Text wrappers from four BottomSheetHeader instances |
| app/components/Views/NetworkSelector/RpcSelectionModal/RpcSelectionModal.tsx | Removed Text wrapper and updated snapshot |
| app/components/Views/NetworkSelector/NetworkSelector.tsx | Removed Text wrapper from delete confirmation modal |
| app/components/Views/AddAsset/components/NetworkListBottomSheet.tsx | Removed Text wrapper and unused imports |
| app/components/Views/AccountPermissions/PermittedNetworksInfoSheet/PermittedNetworksInfoSheet.tsx | Removed Text wrapper and updated snapshot |
| app/components/Views/AccountPermissions/ConnectionDetails/ConnectionDetails.tsx | Removed Text wrapper and updated snapshot |
| app/components/Views/AccountPermissions/AccountPermissionsConfirmRevokeAll/AccountPermissionsConfirmRevokeAll.tsx | Removed Text wrapper and updated snapshot |
| app/components/UI/Ramp/Deposit/Views/Modals/RegionSelectorModal/RegionSelectorModal.tsx | Removed Text wrapper and updated snapshot |
| app/components/UI/Ramp/Deposit/Views/Modals/PaymentMethodSelectorModal/PaymentMethodSelectorModal.tsx | Removed Text wrapper and updated snapshot |
| app/components/UI/NetworkManager/index.tsx | Removed Text wrapper from delete confirmation modal |
| app/components/UI/Bridge/components/TransactionDetails/BlockExplorersModal.tsx | Removed Text wrapper |
| app/components/UI/Bridge/components/SlippageModal/index.tsx | Removed Text wrapper and TextVariant import, updated snapshot |
| app/components/UI/Bridge/components/QuoteExpiredModal/QuoteExpiredModal.tsx | Removed Text wrapper and updated snapshot |
| app/components/UI/Bridge/components/BridgeTokenSelectorBase.tsx | Removed Text wrapper and TextVariant import |
| app/components/UI/Bridge/components/BridgeNetworkSelectorBase.tsx | Removed Text wrapper, unused imports, and styles |
| app/components/UI/Bridge/components/BlockaidModal/BlockaidModal.tsx | Removed Text wrapper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {strings('app_settings.delete')}{' '} | ||
| {showConfirmDeleteModal.networkName}{' '} | ||
| {strings('asset_details.network')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix mixed children so it's a single string
2c87ef1 to
343fa4f
Compare
| jest.mock( | ||
| '../../../component-library/components/BottomSheets/BottomSheetHeader/BottomSheetHeader', | ||
| () => { | ||
| const { View: RNView } = jest.requireActual('react-native'); | ||
| return ({ children }: { children: React.ReactNode }) => ( | ||
| <RNView testID="bottom-sheet-header">{children}</RNView> | ||
| ); | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we are mocking this. This test file has a lot of unneeded mock anti patterns
| expect(mockOnCloseBottomSheet).toHaveBeenCalled(); | ||
| await waitFor(() => { | ||
| expect(getByTestId('bottom-sheet-header')).toBeOnTheScreen(); | ||
| expect(getByTestId('header')).toBeOnTheScreen(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the root test id header from the HeaderBase component. The BottomSheetHeader wasn't build to accept unique data test ids which is a problem
8b891b5 to
ddcd7e7
Compare
|
| </Text> | ||
| {strings('app_settings.delete')}{' '} | ||
| {showConfirmDeleteModal.networkName}{' '} | ||
| {strings('asset_details.network')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Mixed Content Disrupts Header Text Styling
The BottomSheetHeader children contain mixed JSX expressions with string literals instead of a single string. This causes HeaderBase to skip its internal text wrapping logic (which checks typeof children === 'string'), potentially breaking text styling. The fix should use a template literal like in NetworkManager/index.tsx: {`${strings('app_settings.delete')} ${showConfirmDeleteModal.networkName} ${strings('asset_details.network')}`}
wachunei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ramp changes LGTM



Description
Removed
Textcomponent wrappers fromBottomSheetHeaderchildren across the codebase to ensure consistency with the new API pattern whereBottomSheetHeaderhandles text styling internally.This refactoring removes redundant
Textcomponents that were wrapping string content insideBottomSheetHeader. TheBottomSheetHeadercomponent now handles the text styling directly, ensuring a consistent appearance across all bottom sheet headers in the app.Changelog
CHANGELOG entry: null
Related issues
Part of: https://consensyssoftware.atlassian.net/browse/MDP-343
Manual testing steps
Screenshots/Recordings
Before
N/A - Internal refactoring, no visual changes. We can trust the component to handle styling correctly.
After
N/A - Internal refactoring, no visual changes. We can trust the component to handle styling correctly.
Pre-merge author checklist
Pre-merge reviewer checklist
Changes Summary
11 component files updated:
5 test snapshots updated:
Total: 16 files changed, 54 insertions(+), 60 deletions(-)
Note
Refactors BottomSheetHeader to take raw string children across modals/selectors and updates tests/snapshots and small header style expectations.
<Text>wrappers with raw string children inBottomSheetHeaderacross:BlockaidModal,BridgeNetworkSelectorBase,BridgeTokenSelectorBase,QuoteExpiredModal,SlippageModal,TransactionDetails/BlockExplorersModal.PaymentMethodSelectorModal,RegionSelectorModal.AccountPermissionsConfirmRevokeAll,ConnectionDetails,PermittedNetworksInfoSheet.NetworkManager,AddAsset/NetworkListBottomSheet,NetworkSelector.TooltipModal.testID="header-title", center text, and expect fontSize 16.testID="header"instead of mocked header ID; extend icon mocks.headingstyle inNetworkSettingsstyles; align header typography viaBottomSheetHeader.Written by Cursor Bugbot for commit ddcd7e7. This will update automatically on new commits. Configure here.