Welcome screen slim sidebar #16815
Welcome screen slim sidebar #16815
Conversation
WalkthroughThe pull request updates several layout components across the suite. In the Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
| @@ -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
2 times, most recently
from
5cef839 to
af5976a
Compare
…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
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
| @@ -75,7 +77,7 @@ export const Sidebar = () => { | |||
| <ResizableBox | |||
| directions={['right']} | |||
| width={sidebarWidth} | |||
| minWidth={84} | |||
| minWidth={SIDEBAR_MIN_WIDTH} | |||
| maxWidth={600} | |||
There was a problem hiding this comment.
why not add also max width to const?
TODO:
Description
Related Issue
Resolve #13521
Screenshots:
prev
Current