feat(suite): Delete mobile menu #16336
Conversation
3307d88 to
9b1e004
Compare
9b1e004 to
fc4b335
Compare
packages/suite/src/components/suite/layouts/SuiteLayout/SuiteLayout.tsx
Outdated
Show resolved
Hide resolved
fc4b335 to
bceac79
Compare
| sidebarWidthFromRedux: number; | ||
| }) => { | ||
| const [sidebarWidth, setSidebarWidth] = useState<number>(sidebarWidthFromRedux); | ||
| const [contentWidth, setContentWidth] = useState<number | undefined>(undefined); | ||
|
|
||
| const getIsSidebarCollapsed = () => { |
There was a problem hiding this comment.
Isn't isSidebarCollapsed enough?
| import { useSelector } from 'src/hooks/suite'; | ||
|
|
||
| import { useResponsiveContext } from '../../support/suite/ResponsiveContext'; | ||
|
|
||
| const Container = styled.div` |
There was a problem hiding this comment.
This can be redone with Box, including the media query part, and Divider.
| margin-right: -${spacingsPx.md}; | ||
| padding-left: ${spacingsPx.md}; | ||
| padding-right: ${spacingsPx.md}; | ||
| width: calc(100% + ${spacingsPx.xxl}); |
There was a problem hiding this comment.
This is not needed, just set it to auto, I think.
| @@ -56,7 +58,7 @@ const MobilePromoContainer = styled.div` | |||
| ${promoContainerCss} | |||
| justify-content: start; | |||
|
|
|||
| ${variables.SCREEN_QUERY.MOBILE} { | |||
| ${variables.SCREEN_QUERY.BELOW_LAPTOP} { | |||
There was a problem hiding this comment.
Custom css components not needed.
|
Related to these issues: #16401 It should be fixed as well |
bceac79 to
4cd5fe5
Compare
WalkthroughThe update spans multiple components and focuses on state synchronization, responsive design, and layout streamlining. In the Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/components/src/components/ResizableBox/ResizableBox.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/MobileMenu/MobileMenu.tsx(0 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/UpdateNotificationBanner.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/consts.ts(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/SuiteLayout.tsx(2 hunks)packages/suite/src/components/wallet/WalletLayout/AccountsMenu/MobileAccountsMenu.tsx(0 hunks)packages/suite/src/support/suite/ResponsiveContext.tsx(2 hunks)packages/suite/src/views/dashboard/PromoBanner.tsx(5 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/suite/layouts/SuiteLayout/MobileMenu/MobileMenu.tsx
- packages/suite/src/components/wallet/WalletLayout/AccountsMenu/MobileAccountsMenu.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/views/dashboard/PromoBanner.tsx
[error] 8-8: Shouldn't redeclare 'Button'. Consider to delete it or rename it.
'Button' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'Icon'. Consider to delete it or rename it.
'Icon' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'Image'. Consider to delete it or rename it.
'Image' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'Tooltip'. Consider to delete it or rename it.
'Tooltip' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'variables'. Consider to delete it or rename it.
'variables' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'Row'. Consider to delete it or rename it.
'Row' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'Column'. Consider to delete it or rename it.
'Column' is defined here:
(lint/suspicious/noRedeclare)
[error] 9-9: Shouldn't redeclare 'isWeb'. Consider to delete it or rename it.
'isWeb' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (13)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/consts.ts (1)
1-2: LGTM! Good practice to centralize width constraints.Centralizing the sidebar width constraints as constants improves maintainability and makes it easier to adjust these values across the codebase if needed.
packages/suite/src/support/suite/ResponsiveContext.tsx (2)
30-34: Well-encapsulated sidebar collapse logic.The
getIsSidebarCollapsedfunction cleanly encapsulates the collapse logic, making it easier to maintain and modify. The mobile layout takes precedence, ensuring consistent behavior on mobile devices.
36-42:❓ Verification inconclusive
Verify mobile layout behavior.
The context enforces
SIDEBAR_MIN_WIDTHand collapsed state on mobile layouts. Please ensure this doesn't affect any existing features that might depend on a different sidebar width in mobile view.
🏁 Script executed:
#!/bin/bash # Search for components that might depend on specific sidebar widths rg -l "sidebarWidth|isSidebarCollapsed" packages/suite/src/Length of output: 1621
Mobile Layout Sidebar Behavior Verification
The responsive context now enforces
SIDEBAR_MIN_WIDTHand a collapsed state on mobile layouts. Our search confirms that several components—such as those in SuiteLayout (e.g.,SuiteLayout.tsx,Sidebar.tsx, and related DeviceSelector and Navigation components) as well as wallet layouts (e.g.,AccountsMenu.tsxand its subcomponents)—rely onsidebarWidthandisSidebarCollapsed. Please verify that forcingSIDEBAR_MIN_WIDTHin mobile views doesn’t inadvertently affect any component-specific behavior or features that expect a different, more dynamic sidebar width. Testing the mobile UI across these areas is recommended to ensure overall stability.packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx (2)
41-43: LGTM! Clear type definition.The
SidebarPropstype clearly defines the component's requirements.
82-93:❓ Verification inconclusive
Verify ResizableBox behavior with locked state.
The ResizableBox configuration looks good, with proper width constraints and mobile locking. However, please verify that:
- The disabledWidthInterval still makes sense with the locked state
- The resize handlers (handleSidebarWidthChanged, handleSidebarWidthUpdate) handle the locked state gracefully
🏁 Script executed:
#!/bin/bash # Search for ResizableBox usage and configuration ast-grep --pattern 'ResizableBox { $$$ isLocked={$_} $$$ }'Length of output: 67
Action: Manually verify ResizableBox behavior when locked
Please confirm the following in the implementation:
- Ensure that the
disabledWidthInterval={[84, 240]}still makes sense when the sidebar is in a locked state (i.e. whenisMobileLayoutis true).- Verify that the resize event handlers (
handleSidebarWidthChangedandhandleSidebarWidthUpdate) include checks or logic to gracefully handle the locked state without causing unintended width changes.Since the automated AST search did not yield conclusive output, please double-check these aspects in the relevant code and ensure that the locked state is being properly accounted for in both the component's props and its handler logic.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/UpdateNotificationBanner.tsx (1)
96-101: LGTM! Improved visibility logic and formatting.The conditions for hiding the banner are well-organized and include the sidebar collapsed state, which is essential for mobile layouts. The improved formatting makes the conditions more readable.
packages/suite/src/views/dashboard/PromoBanner.tsx (3)
26-37: LGTM! Improved responsive design.The changes enhance the responsive design by:
- Using consistent spacing values from the theme
- Updating media query to use MOBILE breakpoint
- Adjusting container height for better content accommodation
213-213: LGTM! Improved button consistency.The addition of the size prop enhances button consistency by using standard component sizing.
221-243: LGTM! Enhanced mobile layout.The changes improve the mobile layout by:
- Using Column component for better vertical stacking
- Applying consistent spacing from theme
- Setting appropriate width and gap properties
packages/components/src/components/ResizableBox/ResizableBox.tsx (1)
240-250: LGTM! Improved prop synchronization.The new useEffect hooks ensure that the component's internal state stays synchronized with the width and height props, improving the component's reactivity to prop changes.
packages/suite/src/components/suite/layouts/SuiteLayout/SuiteLayout.tsx (3)
35-94: LGTM! Well-structured styled components with proper responsive design.The styled components are well-organized and maintain proper responsive design, including mobile-specific styles for handling overflow behavior.
108-135: LGTM! Clean implementation with proper event handling.The MainContent component follows React best practices with proper event listener management and cleanup.
157-160:❓ Verification inconclusive
Verify complete removal of mobile menu functionality.
The changes look good and align with the PR objectives. The Sidebar is now consistently rendered within ElevationDown, and the mobile menu functionality has been removed.
Let's verify that all mobile menu related code has been completely removed:
Also applies to: 174-176
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining mobile menu related code # Search for MobileMenu references echo "Checking for MobileMenu references..." rg -i "mobilemenu" --type ts --type tsx # Search for mobile-specific menu patterns echo "Checking for mobile-specific menu patterns..." rg -i "mobile.*menu|menu.*mobile" --type ts --type tsxLength of output: 338
I'll generate a new verification script that uses
fdto search for mobile menu references across both.tsand.tsxfiles, bypassing the file type issue encountered withrg. Please run the script below to confirm that all mobile menu related code has indeed been removed:Once you verify that these searches return no results, you can be confident that the mobile menu functionality has been fully removed.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for MobileMenu references in .ts and .tsx files..." fd -e ts -e tsx -x grep -i "MobileMenu" echo "Searching for mobile-specific menu patterns in .ts and .tsx files..." fd -e ts -e tsx -x grep -Ei "mobile.*menu|menu.*mobile"Length of output: 1177
Mobile Menu Removal Verified in Main Code – Please Verify Snapshot Consistency
The updates to the SuiteLayout component correctly remove mobile menu functionality, and our search excluding snapshot directories confirms that no mobile menu references remain in the main source code. However, we did find references (e.g., the
MobileMenuActionscomponent and related comments) in the test snapshot file atpackages/suite-desktop-core/e2e/snapshots/web/wallet/cardano.test.ts. Please verify manually if these snapshot artifacts need updating to reflect the removal or if they’re expected legacy output.
- Main code verification: Mobile menu-related code is absent from active components.
- Test snapshots: References to mobile menu functionality remain; manual review is recommended.
4cd5fe5 to
b33b42e
Compare
|
Please test this on a real Android phone with a smaller screen. Test the onboarding for sure (but the menu is not visible inside onboarding, so it should be fine). It makes the user experience worse, as the menu on the left takes up about a fifth of the screen. Compare for example account selection: AFTER EDIT: In landscape mode, it doesn't look so bad, so landscape mode FTW! |
b33b42e to
b3a0d09
Compare
b3a0d09 to
419461f
Compare
Description
Related Issue
Resolve #15801
Screenshots: