Welcome screen slim sidebar #16815
Welcome screen slim sidebar #16815
Conversation
WalkthroughThe changes update multiple parts of the codebase, including layout components, navigation, and testing utilities. In the suite package, the layout is revised by introducing the Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
2 times, most recently
from
247f430 to
398a375
Compare
| @@ -47,12 +48,16 @@ export const LoggedOutLayout = ({ children }: LoggedOutLayout) => { | |||
| <LayoutContext.Provider value={setLayoutPayload}> | |||
| <Body data-testid="@suite-layout/body"> | |||
| <Columns> | |||
| <ElevationDown> | |||
| <LoggedOutSidebar /> | |||
There was a problem hiding this comment.
@jvaclavik classics said the best: "Je to dobrej nápad, je to špatnej nápad nebo jakej je to nápad?"
I am not sure if this solution is correct. It is pretty elegant for the "logged out state" eg. "welcome" state when user clicks the settings.
But! I can't tell if there are other screen accessible when the user is not logged in.
But overall, it works nice.
There was a problem hiding this comment.
Je to dobrej nápad I think :D
There was a problem hiding this comment.
I checked it with the AI and it doesn't tell me anything sus, but I can't really tell...
| @@ -144,39 +145,33 @@ const NavItem = (props: NavigationItemProps) => { | |||
| $isRounded={isRounded} | |||
| $typographyStyle={typographyStyle} | |||
| > | |||
| <Icon name={icon} size={iconSize} color={theme.iconSubdued} pointerEvents="none" /> | |||
| <ExpandedSidebarOnly> | |||
There was a problem hiding this comment.
I removed the components that needed the context and replaced with passed prop for better reusability.
| @@ -193,7 +130,7 @@ export const WelcomeLayout = ({ children }: WelcomeLayoutProps) => { | |||
|
|
|||
| <Row height="100%" width="100%" data-testid="@welcome-layout/body"> | |||
| <ElevationDown> | |||
| <Left /> | |||
| <LoggedOutSidebar /> | |||
There was a problem hiding this comment.
Since there are multiple layouts for the state when "you are not logged in". I need to include this component here also. Above, it is rendered also in the logged out layout.
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
398a375 to
1137feb
Compare
missing cursor on hover: 
|
different widthwelcome screen: dashboard different elevationwelcome screen: dashboard |
Sidebar on welcome screen is not visible on mobile resolution: 
It shoudn't do anything with sidebar. It's a bit related to this PR #16336 (feel free to take it @vojtatranta ) |
|
It would be nice to fix also bug with changing urls, but I know it's complex topic. Flow in the app:
|
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
1137feb to
8d2aa40
Compare
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx
Show resolved
Hide resolved
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
3 times, most recently
from
af5976a to
cea83a4
Compare
I'd postpone it to the different issue coz this requires a bit of research of how to do it properly. @jvaclavik |
vojtatranta
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx (1)
51-58:🧩 Analysis chain
Verify cursor style on hover.
Based on the PR feedback, there was a reported issue with missing cursor on hover. Please ensure that the
HeaderHeadingcomponent properly displays a pointer cursor on hover since it's clickable.
🏁 Script executed:
#!/bin/bash # Description: Check if HeaderHeading component defines cursor style # Search for cursor style definition in HeaderHeading component rg -A 5 'HeaderHeading.*styled'Length of output: 701
Issue with missing "pointer" cursor on HeaderHeading hover.
The HeaderHeading component—as defined in BasicName.tsx—is missing the "cursor: pointer" style. This confirms that the clickable header does not show the expected pointer cursor on hover. To resolve this, please update the styled component (or its base component) to include "cursor: pointer" on hover.
- In file: packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/BasicName.tsx, add a CSS rule, for example:
cursor: pointer;
🧹 Nitpick comments (6)
packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx (1)
22-22: Consider relocating LoggedOutSidebar component.The
LoggedOutSidebarcomponent is currently imported fromWelcomeLayout, but its name and usage suggest it's a standalone component. Consider moving it to its own file to improve modularity and prevent circular dependencies.packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx (1)
22-27: Add documentation for the settings routes constant.While centralizing the routes improves maintainability, adding documentation would help other developers understand:
- The purpose of these specific routes
- When to add/remove routes from this list
- The relationship between these routes and the settings navigation
Add JSDoc comment above the constant:
+/** + * List of routes available in the settings section. + * Used by the navigation to determine active state and routing. + */ export const SETTINGS_ROUTES: Route['name'][] = [packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx (2)
74-104: Consider extracting navigation items to a constant.The component looks good, but consider extracting the navigation items configuration to a constant for better maintainability.
+const LOGGED_OUT_NAV_ITEMS = [ + { + nameId: 'TR_START', + icon: 'trezorLogo', + goToRoute: 'suite-start', + routes: ['suite-start'], + }, + { + nameId: 'TR_SETTINGS', + icon: 'gearSix', + goToRoute: 'settings-index', + routes: SETTINGS_ROUTES, + }, +] as const; export const LoggedOutSidebar = () => { const { elevation } = useElevation(); return ( <WelcomeWrapper $elevation={elevation}> <ElevationUp> <TrafficLightOffset> <WelcomeNavColumn $elevation={elevation} $minWidth={SIDEBAR_MIN_WIDTH}> <Column justifyContent="space-between" height="100%"> <Nav $isSidebarCollapsed> - <NavItem - nameId="TR_START" - icon="trezorLogo" - goToRoute="suite-start" - routes={['suite-start']} - /> - <NavItem - nameId="TR_SETTINGS" - icon="gearSix" - goToRoute="settings-index" - routes={SETTINGS_ROUTES} - /> + {LOGGED_OUT_NAV_ITEMS.map(item => ( + <NavItem key={item.nameId} {...item} /> + ))} </Nav>
133-133: Consider documenting the dual usage of LoggedOutSidebar.As mentioned in the past review comment, this component is rendered in multiple layouts. Consider adding a code comment explaining this intentional duplication to prevent future confusion.
+ {/* LoggedOutSidebar is intentionally rendered in both WelcomeLayout and LoggedOutLayout */} <LoggedOutSidebar />packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx (1)
40-40: UseSIDEBAR_MIN_WIDTHconsistently throughout the component.While extracting the magic number is good practice, consider using the constant in
disabledWidthIntervalas well to maintain consistency.- disabledWidthInterval={[84, 240]} + disabledWidthInterval={[SIDEBAR_MIN_WIDTH, 240]}packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx (1)
21-49: Document the magic number in the click counter logic.While the code is well-structured, consider documenting why specifically 5 clicks are required to toggle debug mode. This would help future maintainers understand the rationale behind this number.
- // show debug menu item after 5 clicks on "Settings" heading + // Show debug menu item after 5 clicks on "Settings" heading. + // Note: 5 clicks are used as a "secret" gesture to prevent accidental activation + // while making it discoverable enough for developers who need it. const [clickCounter, setClickCounter] = useState<number>(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: e2e-test-suite-web (@group=other, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata2, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata1, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=suite, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group_wallet, trezor-user-env-unix bitcoin-regtest, 1)
- GitHub Check: e2e-test-suite-web (@group_passphrase, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_device-management, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_suite, trezor-user-env-unix, 1)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- 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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (15)
packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx (1)
51-53: Verify elevation consistency across layouts.The implementation of
ElevationDownfor the sidebar aligns with the broader elevation management changes. However, ensure this elevation level is consistent with other layouts in the logged-out state.Let's verify the elevation management implementation across layouts:
#!/bin/bash # Search for ElevationDown usage in layout components rg -A 2 "ElevationDown" "packages/suite/src/components/suite/layouts/"packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx (3)
5-5: LGTM!Clean import of the
Routetype for proper TypeScript typing.
12-12: LGTM!Exporting the
Navcomponent promotes reusability across layouts, which aligns with the PR's objective of implementing the slim sidebar in different views.
45-45: LGTM!Good use of the centralized
SETTINGS_ROUTESconstant, which reduces duplication and improves maintainability.packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx (2)
14-20: LGTM! Well-organized imports.The imports are properly organized and necessary for the new functionality.
Also applies to: 29-32
68-72: LGTM! Well-structured styled component.The component is well-typed and follows the design system patterns.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (3)
11-24: LGTM! Well-structured styled component.The implementation follows styled-components best practices with proper use of transient props and theme variables.
26-29: LGTM! Type definition properly reflects component requirements.The type changes appropriately handle required and optional props.
31-44: LGTM! Clean and type-safe implementation.The component follows React best practices with proper prop handling and explicit boolean conversion.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx (2)
80-80: LGTM! Good use of the constant.Using the constant improves maintainability.
104-107: LGTM! Props properly passed from context.The component correctly handles both the new required prop and existing functionality.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (3)
78-78: LGTM! The expanded prop addition aligns with the slim sidebar implementation.The optional expanded prop maintains backward compatibility while enabling the new slim sidebar functionality.
174-176: LGTM! The component is now more reusable.The implementation correctly uses the responsive context and passes the expanded state to NavItem, improving reusability as discussed in the past review.
147-168: Tooltip implementation looks good but verify cursor behavior.The tooltip implementation correctly handles the expanded state and follows React best practices. The cursor style is set to "pointer" as requested in the past review.
Please verify the cursor behavior matches the design system by checking if:
- The cursor changes to pointer on hover
- The tooltip appears with the correct styling when hovering over collapsed items
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for cursor-related styles in the theme or components rg -A 2 'cursor:' packages/suite/src/componentsLength of output: 15224
NavigationItem Cursor Behavior Verified
- The NavigationItem component sets “cursor: pointer” (as seen in its own styles) and passes it to the Tooltip.
- This is consistent with similar components in the codebase, ensuring that on hover the cursor changes to pointer for collapsed items.
- The tooltip appears correctly based on the “expanded” state.
packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx (1)
1-19: LGTM!The imports are well organized and the styled component follows best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite-desktop-core/e2e/support/bridge.ts (1)
17-18: Update comment to reflect new selector.The comment should be updated to reference
@welcome-layout/bodyinstead of the old selector for consistency.-// We wait for `@welcome-layout/body` or `@dashboard/graph` since +// We wait for either `@welcome-layout/body` or `@dashboard/graph` sincepackages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts (1)
117-117: Consider extracting the visibility check to a helper method.This visibility check is duplicated from
optionallyDismissFwHashCheckError. Consider extracting it to a private helper method to improve maintainability.export class OnboardingActions { + private async waitForWelcomeBody() { + await expect(this.welcomeBody).toBeVisible({ timeout: 10000 }); + } + @step() async optionallyDismissFwHashCheckError() { - await expect(this.welcomeBody).toBeVisible({ timeout: 10000 }); + await this.waitForWelcomeBody(); // dismisses the error modal only if it appears... } @step() async disableFirmwareHashCheck() { if (!isWebProject(this.testInfo)) { return; } - await expect(this.welcomeBody).toBeVisible({ timeout: 10000 }); + await this.waitForWelcomeBody(); // eslint-disable-next-line @typescript-eslint/no-shadow await this.page.evaluate(SuiteActions => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (8)
packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts (1)
8-8: LGTM! Selector update aligns with UI changes.The update from
welcomeTitletowelcomeBodycorrectly reflects the new UI structure while maintaining the same timeout value.packages/suite-desktop-core/e2e/tests/browser/safari.test.ts (1)
32-32: LGTM! Consistent selector update.The selector change maintains consistency with other test files while preserving the test's comprehensive validation of Safari's unsupported state.
packages/suite-desktop-core/e2e/support/bridge.ts (1)
23-23: LGTM! Selector update maintains function behavior.The selector change is consistent with the new UI structure while preserving the function's race condition handling.
packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts (2)
58-58: LGTM! Selector update in happy path test.The selector change maintains consistency with the new UI structure.
86-86: LGTM! Selector update in Tor request verification test.The selector change is consistent while maintaining the test's purpose of verifying Tor routing.
packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts (3)
21-21: LGTM!The property declaration is correct with appropriate type and immutability.
86-86: LGTM!The visibility check with a 10-second timeout is appropriate for this e2e test scenario.
55-55: Verify the test ID exists in the welcome layout component.The test ID '@welcome-layout/body' suggests targeting a broader layout element. Ensure this test ID is properly set in the corresponding React component.
| @@ -47,12 +48,16 @@ export const LoggedOutLayout = ({ children }: LoggedOutLayout) => { | |||
| <LayoutContext.Provider value={setLayoutPayload}> | |||
| <Body data-testid="@suite-layout/body"> | |||
| <Columns> | |||
| <ElevationDown> | |||
| <LoggedOutSidebar /> | |||
There was a problem hiding this comment.
Je to dobrej nápad I think :D
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
Show resolved
Hide resolved
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
3d48a6e to
1748527
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (1)
147-168: Fix tooltip behavior and cursor style.The tooltip implementation has two issues:
- The cursor style should inherit from the parent instead of being fixed to "pointer".
- The tooltip should be disabled when the sidebar is expanded to prevent incorrect behavior.
Apply this diff to fix the issues:
- <Tooltip - cursor="pointer" - content={<Title nameId={nameId} values={values} />} - isActive={!expanded} - placement="right" - hasArrow - > + <Tooltip + content={<Title nameId={nameId} values={values} />} + isActive={!expanded} + placement="right" + hasArrow + >
🧹 Nitpick comments (1)
packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx (1)
125-135: Consider simplifying nested elevation components.There are nested
ElevationDowncomponents (one at line 125 and another at line 133). This might lead to unintended elevation stacking effects.Consider removing the inner
ElevationDownwrapper since the outer one should suffice:<Row height="100%" width="100%" data-testid="@welcome-layout/body"> - <ElevationDown> <LoggedOutSidebar /> - </ElevationDown>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (7)
packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx (2)
14-20: LGTM! Well-structured imports and styled component.The new imports and the
WelcomeNavColumnstyled component are well-organized and properly utilize the theme system.Also applies to: 28-32, 68-72
74-105: LGTM! Clean implementation of the slim sidebar.The
LoggedOutSidebarcomponent effectively implements the slim sidebar with proper navigation structure and elevation management. The reuse of this component across different layouts is a good practice.packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
26-29: LGTM! Props interface changes look good.The updated props interface correctly reflects the component's requirements, with
isSidebarCollapsedas a required prop andshowUpdateBannerNotificationas optional.
31-44: LGTM! Component implementation looks good.The changes improve type safety with explicit boolean conversion and simplify the code with implicit return.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (3)
21-21: LGTM! Import looks good.The
useResponsiveContextimport is necessary for managing the sidebar state.
78-78: LGTM! Props interface change looks good.The optional
expandedprop is correctly added to support the slim sidebar feature.
173-177: LGTM! Component implementation looks good.The component correctly uses the responsive context to manage the sidebar state.
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
1748527 to
61233e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx (1)
22-27: Add documentation for the SETTINGS_ROUTES constant.Consider adding a JSDoc comment to document the purpose and usage of this constant, as it's now a central source of truth for settings routes.
+/** + * List of valid route names for the settings section. + * Used to centralize settings navigation configuration. + */ export const SETTINGS_ROUTES: Route['name'][] = [ 'settings-index', 'settings-device', 'settings-coins', 'settings-debug', ] as const;packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx (1)
74-106: LGTM! Consider adding JSDoc documentation.The LoggedOutSidebar implementation is clean and well-structured. The navigation items and quick actions are properly integrated with the slim sidebar design.
Consider adding JSDoc documentation to explain the component's purpose and its relationship with other layouts:
+/** + * LoggedOutSidebar component renders a slim sidebar for non-authenticated routes. + * It shares the same navigation structure as the main sidebar but remains in a collapsed state. + */ export const LoggedOutSidebar = () => {packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
31-44: Consider adding documentation for quick actions.The implementation is clean and follows best practices. Consider adding a brief comment explaining the purpose of each quick action component for better maintainability.
Add documentation above the component:
+/** + * QuickActions component renders a collection of action buttons in the sidebar: + * - UpdateStatusActionBarIcon: Displays update notifications + * - DebugAndExperimental: Toggle for debug features + * - CustomBackend: Backend configuration options + * - Tor: Tor network settings + * - HideBalances: Toggle for balance visibility + */ export const QuickActions = ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-web
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (8)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx (3)
5-5: LGTM!The
Routetype import is appropriately added for type safety.
12-12: LGTM!The
Navcomponent is now properly exported for reuse, which aligns with the PR's objective of implementing the slim sidebar feature.
45-45: LGTM!Using the centralized
SETTINGS_ROUTESconstant improves maintainability and type safety.packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx (2)
14-32: LGTM! Well-structured imports and styled component.The new imports and styled component are well-organized, utilizing the theme system effectively and maintaining consistency with the sidebar width constant.
Also applies to: 68-72
126-145: LGTM! Clean layout structure.The elevation hierarchy and layout structure are well-implemented. The inclusion of LoggedOutSidebar is justified by the need to support multiple non-authenticated layouts, as mentioned in the past review comments.
packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts (1)
90-90: Verify the new navigation flow in tests.The locator change from '@settings/menu/close' to '@suite/menu/suite-start' reflects a UX change where settings are now closed via the suite start menu item instead of a dedicated close button. While this aligns with the slim sidebar implementation, let's verify the change maintains expected test behavior.
Run this script to verify the navigation flow in tests:
Consider adding a comment in the
closeSettings()method to document this navigation change for future test maintenance.@step() async closeSettings() { + // Settings are closed by navigating to suite start menu item await this.settingsCloseButton.click(); await this.settingsHeader.waitFor({ state: 'detached' }); }Also applies to: 140-144
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the new locator is consistently used and tests are passing # Search for any remaining references to old locator echo "Checking for old locator references..." rg -l "@settings/menu/close" # Search for test files using closeSettings() echo "Finding test files using closeSettings()..." rg -l "closeSettings\(\)" # Search for related test failures echo "Checking for related test failures..." rg -l "Error.*@suite/menu/suite-start" || echo "No failures found"Length of output: 554
The locator update maintains test functionality as expected.
- The old locator (
@settings/menu/close) is no longer referenced in the codebase.- All tests referencing
closeSettings()are functioning correctly without errors.- No related error logs for
@suite/menu/suite-startwere found.packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Well-structured styled component.The styled component follows best practices with proper use of theme variables, transient props, and responsive design patterns.
26-29: LGTM! Clear and well-defined props interface.The type definition properly distinguishes between required and optional props with clear naming.
packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
Show resolved
Hide resolved
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
2 times, most recently
from
3b1f5b3 to
a396e93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (1)
147-168: LGTM! Tooltip behavior is correctly implemented.The tooltip is now properly disabled when the sidebar is expanded, and the cursor style is consistent.
Consider extracting the expanded content into a separate component for better readability:
- {expanded && ( - <> - <Paragraph typographyStyle={typographyStyle}> - <Translation id={nameId} values={values} /> - </Paragraph> - - {itemsCount && ( - <Paragraph variant="tertiary" typographyStyle={typographyStyle}> - {itemsCount} - </Paragraph> - )} - </> - )} + {expanded && <ExpandedContent {...props} />} +const ExpandedContent = ({ nameId, values, itemsCount, typographyStyle }: NavigationItemProps) => ( + <> + <Paragraph typographyStyle={typographyStyle}> + <Translation id={nameId} values={values} /> + </Paragraph> + + {itemsCount && ( + <Paragraph variant="tertiary" typographyStyle={typographyStyle}> + {itemsCount} + </Paragraph> + )} + </> +);packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
31-44: Consider performance and default value optimizations.While the implementation is correct, consider these improvements:
- Move the boolean conversion to prop destructuring
- Memoize the component to optimize re-renders
-export const QuickActions = ({ +const defaultProps = { + showUpdateBannerNotification: false, +}; + +export const QuickActions = React.memo(({ isSidebarCollapsed, - showUpdateBannerNotification, -}: QuickActionsProps) => ( + showUpdateBannerNotification = defaultProps.showUpdateBannerNotification, +}: QuickActionsProps) => ( <ActionsContainer $isSidebarCollapsed={isSidebarCollapsed}> <UpdateStatusActionBarIcon - showUpdateBannerNotification={Boolean(showUpdateBannerNotification)} + showUpdateBannerNotification={showUpdateBannerNotification} /> <DebugAndExperimental /> <CustomBackend /> <Tor /> <HideBalances /> </ActionsContainer> -); +)); + +QuickActions.displayName = 'QuickActions';Don't forget to add the React import at the top:
import React from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite-desktop-core/e2e/support/bridge.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-web
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (5)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (2)
75-91: LGTM! Interface changes look good.The optional
expandedprop is well-typed and properly integrated into the component's props interface.
173-177: LGTM! Responsive behavior is well implemented.The component correctly uses the responsive context to control the expanded state.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Well-structured styled component.The styled component follows best practices with proper use of transient props, theme variables, and responsive layout handling.
26-29: LGTM! Clear and well-defined props interface.The type definition properly reflects the component's requirements with
isSidebarCollapsedas a required prop andshowUpdateBannerNotificationas optional.packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx (1)
89-91: Consider adding test cases for new state slices.The additions of
blockchainanddesktopUpdatestate slices look good. However, consider adding test cases to verify how the Preloader component behaves with different values in these new state slices.Let's check if there are any existing test cases for these state slices:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for test cases related to blockchain and desktopUpdate states # Test: Look for test cases that use these state slices rg -A 5 "blockchain:|desktopUpdate:" "**/*.test.{ts,tsx}"Length of output: 118
I'll run a new script to specifically check for any usage of these new state slices within the test files (in particular within the Preloader test) to confirm if there are any test cases for them.
🏁 Script executed:
#!/bin/bash # Verify the occurrences of the new state slices in the Preloader test file rg "blockchain:" packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx -n rg "desktopUpdate:" packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx -nLength of output: 268
Add test cases for the new state slices
The Preloader test file now properly integrates the
blockchainanddesktopUpdatestate slices. However, no test scenarios are present to verify component behavior with different values for these slices. Consider adding tests that simulate various conditions for these states to ensure the component reacts appropriately.
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
fe96367 to
6c15e4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
31-44: Consider memoizing the component for performance.The implementation looks good, but since this component is part of the layout and might receive frequent prop updates (especially
isSidebarCollapsed), consider wrapping it withReact.memoto prevent unnecessary re-renders.-export const QuickActions = ({ +export const QuickActions = React.memo(({ isSidebarCollapsed, showUpdateBannerNotification, -}: QuickActionsProps) => ( +}: QuickActionsProps) => ( <ActionsContainer $isSidebarCollapsed={isSidebarCollapsed}> <UpdateStatusActionBarIcon showUpdateBannerNotification={Boolean(showUpdateBannerNotification)} /> <DebugAndExperimental /> <CustomBackend /> <Tor /> <HideBalances /> </ActionsContainer> -); +));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
🔇 Additional comments (5)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Clean and responsive styling implementation.The styled component effectively handles both expanded and collapsed states of the sidebar using flex layout, with proper use of theme variables for consistent styling.
26-29: LGTM! Type changes align with component requirements.The type definition properly reflects the component's needs with:
- Required
isSidebarCollapsedprop for layout control- Optional
showUpdateBannerNotificationprop that's safely handled in the implementationpackages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (3)
21-21: LGTM!The new import and prop addition are well-structured and properly typed.
Also applies to: 78-78
147-168: Verify tooltip behavior in expanded state.Based on past review comments, there were concerns about incorrect tooltip display in expanded state. The current implementation uses
isActive={!expanded}to control this, but we should verify this works as expected.Let's verify the tooltip behavior:
#!/bin/bash # Search for similar tooltip implementations in the codebase ast-grep --pattern $'<Tooltip $$$ isActive={!$_} $$$ >'
173-177: LGTM!The component correctly uses the responsive context to control the sidebar state, aligning with the PR objectives.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx
Show resolved
Hide resolved
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
6c15e4a to
fbe4cac
Compare
related issues here: |
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
fbe4cac to
d6de18f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
26-29: Consider adding JSDoc comments to document the props.While the type definitions are correct, adding JSDoc comments would improve code maintainability by documenting the purpose of each prop.
type QuickActionsProps = { + /** Controls the layout direction of quick actions based on sidebar state */ isSidebarCollapsed: boolean; + /** Optional flag to show update notification in the banner */ showUpdateBannerNotification?: boolean; };packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (1)
156-168: Optimize conditional rendering performance.The expanded content could be memoized to prevent unnecessary re-renders.
Consider extracting and memoizing the expanded content:
+const ExpandedContent = memo(({ + nameId, + values, + itemsCount, + typographyStyle +}: Pick<NavigationItemProps, 'nameId' | 'values' | 'itemsCount' | 'typographyStyle'>) => ( + <> + <Paragraph typographyStyle={typographyStyle}> + <Translation id={nameId} values={values} /> + </Paragraph> + {itemsCount && ( + <Paragraph variant="tertiary" typographyStyle={typographyStyle}> + {itemsCount} + </Paragraph> + )} + </> +)); + {expanded && ( - <> - <Paragraph typographyStyle={typographyStyle}> - <Translation id={nameId} values={values} /> - </Paragraph> - {itemsCount && ( - <Paragraph variant="tertiary" typographyStyle={typographyStyle}> - {itemsCount} - </Paragraph> - )} - </> + <ExpandedContent + nameId={nameId} + values={values} + itemsCount={itemsCount} + typographyStyle={typographyStyle} + /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=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: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (5)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Well-structured styled component.The styled component implementation follows best practices with proper use of transient props, theme variables, and flexible layout that adapts to both expanded and collapsed states.
31-44: LGTM! Clean and efficient implementation.The component implementation is well-structured with:
- Proper prop handling
- Explicit boolean conversion for the notification prop
- Clean component composition
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (3)
21-21: LGTM! Clean interface extension.The addition of the
useResponsiveContexthook and the optionalexpandedprop aligns well with the slim sidebar implementation.Also applies to: 78-78
147-155: Fix tooltip cursor and pointer events.The tooltip implementation has interaction issues that were previously identified:
- The tooltip's
cursor="pointer"conflicts with the container's cursor style- The icon's
pointerEvents="none"interferes with tooltip interactionApply this diff to fix the issues:
<Tooltip - cursor="pointer" content={<Title nameId={nameId} values={values} />} isActive={!expanded} placement="right" hasArrow > - <Icon name={icon} size={iconSize} color={theme.iconSubdued} pointerEvents="none" /> + <Icon name={icon} size={iconSize} color={theme.iconSubdued} /> </Tooltip>
173-177: LGTM! Clean integration with responsive context.The
NavigationItemcomponent correctly uses the responsive context to manage the sidebar state and properly passes the expanded state toNavItem.
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
d6de18f to
5287f3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (1)
147-155:⚠️ Potential issueFix tooltip cursor and pointer event handling.
The current tooltip implementation has interaction issues that were previously identified:
- Setting
cursor="pointer"on the tooltip may conflict with container styles- Using
pointerEvents="none"on the icon prevents proper interactionApply this diff to fix the issues:
<Tooltip - cursor="pointer" content={<Title nameId={nameId} values={values} />} isActive={!expanded} placement="right" hasArrow > - <Icon name={icon} size={iconSize} color={theme.iconSubdued} pointerEvents="none" /> + <Icon name={icon} size={iconSize} color={theme.iconSubdued} /> </Tooltip>
🧹 Nitpick comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
31-44: Consider simplifying the boolean conversion.The implementation looks good, but the explicit boolean conversion could be simplified.
- showUpdateBannerNotification={Boolean(showUpdateBannerNotification)} + showUpdateBannerNotification={!!showUpdateBannerNotification}The
!!operator is a more concise way to convert a value to boolean in TypeScript/JavaScript.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- 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: Analyze with CodeQL (javascript)
🔇 Additional comments (5)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Well-structured styled component.The
ActionsContainerstyled component is well-implemented with:
- Proper use of transient props
- Consistent theme usage
- Responsive layout handling
26-29: LGTM! Props interface is well-defined.The updated
QuickActionsPropstype properly reflects the component's requirements with clear prop definitions.packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (3)
21-21: LGTM! Clean interface update.The new
expandedprop and responsive context integration align well with the slim sidebar implementation.Also applies to: 78-78
156-168: LGTM! Clean conditional rendering.The expanded state implementation is well-structured with consistent typography styling and proper handling of optional item counts.
173-177: LGTM! Clean responsive integration.The component effectively uses the responsive context to manage sidebar state, improving maintainability by removing direct context dependencies.
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
5287f3a to
84c853c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
31-44: Consider simplifying the boolean conversion.The implementation looks good, but the boolean conversion could be simplified.
- showUpdateBannerNotification={Boolean(showUpdateBannerNotification)} + showUpdateBannerNotification={!!showUpdateBannerNotification}This change:
- Is more idiomatic in TypeScript/JavaScript
- Achieves the same result with less verbosity
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Well-structured styled component.The styled component implementation follows best practices with transient props, proper theme usage, and responsive layout handling.
26-29: LGTM! Props interface updated appropriately.The type changes align well with the component's requirements:
- Required
isSidebarCollapsedprop ensures proper sidebar state handling- Optional
showUpdateBannerNotificationprovides flexibility
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
2 times, most recently
from
57e8699 to
4fb52a7
Compare
|
QA instruct: pls, check if the "new slim sidebar" works as expected in the "logged out state". We weren't exactly sure (and AI didn't help us much) what are alll teh cases where the "logged out layout" is being displayed. The thing is that this new Sidebar was put to all page where "logged out layout" is displayed. So we need another person to look at it to just rule out some obvious wrong cases. Other than that, please check if the links and buttons work as expected and pls, try also a the electron app. There are some additional buttons, these should work properly, but more eyes see more. Many thanks |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (1)
147-155:⚠️ Potential issueFix tooltip cursor and pointer event settings.
The tooltip implementation has potential interaction issues:
- Setting
cursor="pointer"on the tooltip may conflict with the container's cursor style.- Using
pointerEvents="none"on the icon might interfere with tooltip interaction.Apply this diff to fix the issues:
<Tooltip - cursor="pointer" content={<Title nameId={nameId} values={values} />} isActive={!expanded} placement="right" hasArrow > - <Icon name={icon} size={iconSize} color={theme.iconSubdued} pointerEvents="none" /> + <Icon name={icon} size={iconSize} color={theme.iconSubdued} /> </Tooltip>
🧹 Nitpick comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx (1)
24-49: Consider improving debug mode activation UX.The debug mode activation through 5 clicks is not discoverable. Consider:
- Extracting the magic number 5 to a named constant
- Adding a tooltip or visual feedback as clicks progress
+const DEBUG_MODE_CLICK_THRESHOLD = 5; + export const SettingsName = () => { const isDebugModeActive = useSelector(selectIsDebugModeActive); // show debug menu item after 5 clicks on "Settings" heading const [clickCounter, setClickCounter] = useState<number>(0); const dispatch = useDispatch(); const handleTitleClick = () => { setClickCounter(prev => prev + 1); - if (clickCounter === 4) { + if (clickCounter === DEBUG_MODE_CLICK_THRESHOLD - 1) { setClickCounter(0); dispatch(setDebugMode({ showDebugMenu: !isDebugModeActive }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (5)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (3)
21-21: LGTM! Clean implementation of responsive sidebar state.The addition of
useResponsiveContextand theexpandedprop provides a clean way to manage the sidebar's state.Also applies to: 78-78
156-168: LGTM! Clean implementation of expanded content.The conditional rendering of expanded content is well-structured and handles optional items count appropriately.
173-177: LGTM! Clean implementation of responsive navigation.The
NavigationItemcomponent cleanly integrates with the responsive context to manage sidebar state.packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx (2)
11-11: LGTM!The import is properly organized and necessary for the debug mode functionality.
51-57:❓ Verification inconclusive
Verify E2E test coverage after back button removal.
The removal of the back button aligns with the PR objectives. However, since this previously caused E2E test issues, ensure that the test suite has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining E2E test references to the removed back button # Search for potential test selectors or references rg -i "settings.*back|back.*settings" -g "*.e2e.ts" -g "*.e2e.js"Length of output: 216
E2E Test Verification Needed for Back Button Removal
The intentional removal of the back button in
SettingsName.tsx(lines 51–57) aligns with the slim sidebar objectives set out in the PR. However, previous issues arose when residual back button references broke E2E tests. Although an automated search using file filters for.e2e.tsand.e2e.jsfiles returned no matches, the output suggests that the filter might be excluding some test files (e.g. those with naming conventions such as.spec.js,.test.ts, etc.).
- Action Required: Please manually verify that no test files contain outdated references or selectors related to the back button. This includes reviewing test files beyond the specified globs (like
.specor.testfiles) to ensure that all E2E tests have been updated to reflect this UI change.
…or of left slimd sidebar
…ser is not logged in (replaced by the sidebar)
vojtatranta
force-pushed
the
13521-welcome-screen-slim-sidebar
branch
from
4fb52a7 to
cc37307
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (1)
31-44: Consider memoizing the component for performance.While the implementation is clean and follows best practices, consider wrapping the component with
React.memosince it's likely rendered frequently as part of the sidebar navigation.-export const QuickActions = ({ +export const QuickActions = React.memo(({ isSidebarCollapsed, showUpdateBannerNotification, -}: QuickActionsProps) => ( +}: QuickActionsProps) => ( <ActionsContainer $isSidebarCollapsed={isSidebarCollapsed}> <UpdateStatusActionBarIcon showUpdateBannerNotification={Boolean(showUpdateBannerNotification)} /> <DebugAndExperimental /> <CustomBackend /> <Tor /> <HideBalances /> </ActionsContainer> -); +));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/support/bridge.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(4 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(1 hunks)packages/suite/src/components/suite/Preloader/__tests__/Preloader.test.tsx(2 hunks)packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx(4 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx(3 hunks)packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx(5 hunks)packages/suite/src/reducers/suite/desktopUpdateReducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/suite-desktop-core/e2e/tests/browser/safari.test.ts
- packages/suite/src/reducers/suite/desktopUpdateReducer.ts
- packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts
- packages/suite-desktop-core/e2e/tests/browser/firefox.test.ts
- packages/suite-desktop-core/e2e/support/bridge.ts
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Sidebar.tsx
- packages/suite/src/components/suite/layouts/LoggedOutLayout.tsx
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts
- packages/suite/src/components/suite/Preloader/tests/Preloader.test.tsx
- packages/suite/src/components/suite/layouts/WelcomeLayout/WelcomeLayout.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/PageHeader/PageNames/SettingsName.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (6)
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/NavigationItem.tsx (4)
78-78: LGTM: Well-typed interface extension.The optional
expandedproperty is correctly typed and aligns with the component's new functionality.
147-155: Tooltip cursor and pointer events need adjustment.The current implementation has potential interaction issues:
- The tooltip's
cursor="pointer"may conflict with the container's cursor style.- The icon's
pointerEvents="none"might interfere with tooltip interaction.
156-168: LGTM: Clean conditional rendering implementation.The expanded state handling is well-implemented:
- Shows tooltip only when sidebar is collapsed
- Properly renders content with optional item count when expanded
173-177: LGTM: Clean integration with responsive context.The component efficiently:
- Uses the responsive context to determine sidebar state
- Minimizes prop drilling by passing only necessary props
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx (2)
11-24: LGTM! Well-structured styled component.The
ActionsContainerimplementation follows styled-components best practices with transient props and theme usage. The responsive layout adapts well to the sidebar state.
26-29: LGTM! Props interface is well-defined.The type changes appropriately reflect the component's requirements, with clear distinction between required and optional props.
TODO:
Description
Related Issue
Resolve #13521
Screenshots:
prev
Current