fix: sidebar navigation issues on mobile/tablet viewports#2485
fix: sidebar navigation issues on mobile/tablet viewports#2485AbhiVarde wants to merge 3 commits intoappwrite:mainfrom
Conversation
Console (appwrite/console)Project ID: Sites (2)
Tip Teams feature lets you group users with membership management and role permissions |
WalkthroughIntroduces an optional exported prop Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you @AbhiVarde! @ItzNotABug Could you review this? From a design perspective it looks like a good fix |
AbhiVarde
force-pushed
the
fix-2484-mobile-sidebar-navigation
branch
from
928c68d to
8599f88
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/sidebar.svelte (1)
279-314: Critical: Unreachable else-if block prevents Feedback/Support buttons from rendering.The condition at line 279
{:else if project && $isSmallViewport}can never execute because it's an else-if branch to the{#if project}block starting at line 172. When this else-if is reached,projectis guaranteed to be falsy, makingproject && $isSmallViewportalways false.This means the Feedback and Support buttons inside this block will never render on small viewports when a project exists, which contradicts the PR objective to show these buttons on mobile/tablet.
Review the conditional logic and restructure. Based on the PR objectives and the similar pattern at line 337, this block likely should render when
$isSmallViewportis true (without the redundantprojectcheck), or the entire conditional chain needs refactoring. For example:- {:else if project && $isSmallViewport} + {/if} + {#if project && $isSmallViewport} <div class="action-buttons">Or integrate this logic differently based on your intended behavior. Verify that the conditions at lines 172-314 correctly cover all intended rendering cases for projects on different viewport sizes.
🧹 Nitpick comments (1)
src/lib/components/sidebar.svelte (1)
337-365: Simplify the redundant condition.The condition
(project || (!project && organizationId))is logically equivalent to(project || organizationId)since!projectis implicit whenprojectis falsy.Apply this diff to simplify:
- {#if (project || (!project && organizationId)) && $isTabletViewport} + {#if (project || organizationId) && $isTabletViewport}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/sidebar.svelte(10 hunks)src/lib/layout/shell.svelte(2 hunks)src/routes/(console)/wizard/feedback/mobileFeedbackModal.svelte(1 hunks)src/routes/(console)/wizard/support/mobileSupportModal.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/wizard/support/mobileSupportModal.svelte
🧰 Additional context used
🪛 ESLint
src/lib/components/sidebar.svelte
[error] 279-279: This branch can never execute. Its condition is a duplicate or covered by previous conditions in the {#if} / {:else if} chain.
(svelte/no-dupe-else-if-blocks)
🔇 Additional comments (7)
src/routes/(console)/wizard/feedback/mobileFeedbackModal.svelte (1)
22-25: LGTM! Commenting out the dark theme override aligns with the fix.Disabling the dark theme background override is consistent with the PR objective to remove extra background overlays from modals. The light theme styling remains active, and this change helps resolve rendering conflicts.
src/lib/layout/shell.svelte (2)
178-178: LGTM! Reactive declaration follows the established pattern.The
isOrganizationPagereactive variable correctly mirrors the pattern used forisProjectPageand enables organization-aware layout behavior.
210-213: LGTM! Sidebar now renders on organization pages with proper context.The updated condition ensures the sidebar displays on both project and organization pages, and correctly passes the
organizationIdprop to enable organization-specific UI elements (like the Upgrade button).src/lib/components/sidebar.svelte (4)
57-57: LGTM! organizationId prop properly added.The new optional
organizationIdprop extends the Sidebar API to support organization-aware rendering, enabling features like the Upgrade button on organization pages.Also applies to: 67-67
133-147: LGTM! Upgrade button correctly rendered for organization pages.The implementation properly guards the Upgrade button to display only on organization pages (when
!project && organizationId), includes appropriate analytics tracking, and links to the correct plan change route.
369-372: LGTM! Moving modals outside DropList resolves overlay conflicts.Rendering the mobile modals at the sidebar root level (outside both
Sidebar.BaseandDropListcomponents) prevents the duplicate overlays and rendering conflicts described in the PR objectives. This is the correct approach to fix the modal issues on mobile/tablet viewports.
399-407: LGTM! CSS properly styles the upgrade button container.The styles ensure the Upgrade button anchor fills the container width and centers the text, providing a consistent layout for the organization page upgrade UI.
- Add organization page sidebar support for mobile/tablet - Display upgrade button on organization pages - Fix Settings button visibility on project pages - Show Feedback/Support buttons on all mobile/tablet sizes - Move modals outside DropList to prevent overlay conflicts - Ensure consistent sidebar behavior across viewports Fixes appwrite#2484
AbhiVarde
force-pushed
the
fix-2484-mobile-sidebar-navigation
branch
from
29a7d4d to
cb1864e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/components/sidebar.svelte (1)
257-277: Remove redundant condition check.The
{#if project}check at line 257 is redundant because this code is already inside the{#if project}block that starts at line 172. The outer condition already guarantees thatprojectis truthy.Apply this diff to remove the redundant check:
- {#if project} <div class="mobile-tablet-settings"> <Tooltip placement="right" disabled={state !== 'icons'}> <a href={`${base}/project-${project.region}-${project.$id}/settings`} on:click={() => { trackEvent('click_menu_settings'); sideBarIsOpen = false; }} class="link" class:active={isSelected('/settings') && !isSelected('sites')} ><span class="link-icon"><Icon icon={IconCog} size="s" /></span ><span class:no-text={state === 'icons'} class:has-text={state === 'open'} class="link-text">Settings</span ></a> <span slot="tooltip">Settings</span> </Tooltip> </div> - {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/sidebar.svelte(10 hunks)src/lib/layout/shell.svelte(2 hunks)src/routes/(console)/wizard/feedback/mobileFeedbackModal.svelte(1 hunks)src/routes/(console)/wizard/support/mobileSupportModal.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/(console)/wizard/feedback/mobileFeedbackModal.svelte
- src/lib/layout/shell.svelte
🔇 Additional comments (6)
src/lib/components/sidebar.svelte (5)
57-57: LGTM!The
organizationIdprop is properly typed as optional and follows the existing patterns in the component.Also applies to: 67-67
133-147: LGTM!The upgrade button implementation for organization pages is well-structured with proper analytics tracking and conditional rendering. The button correctly appears only when on an organization page (no project context).
280-315: LGTM!The condition correctly restricts Feedback and Support buttons to project contexts on small viewports. Moving the modal components out of the DropList slots (as shown by the commented code) is the correct approach to fix the overlay issues mentioned in the PR objectives.
370-373: LGTM! Key fix for modal overlay issues.Moving the modal components outside the DropList and rendering them at the sidebar root level correctly addresses the duplicate overlay and rendering conflict issues mentioned in the PR objectives. The tablet viewport conditional ensures these mobile-specific modals are only rendered when needed.
400-408: LGTM!The CSS styling for the upgrade button container is appropriate, ensuring the button fills the width and centers the text for a clean presentation.
src/routes/(console)/wizard/support/mobileSupportModal.svelte (1)
24-26: Dark theme background removal is consistent across both support and feedback modals; verify this matches design intent.The commented-out dark theme rule at lines 24-26 follows the same pattern as
mobileFeedbackModal.svelte, where the dark theme background override is also commented out. Since the Modal component doesn't provide automatic dark mode styling, both modals will display with a white background (#fff) in dark mode. Confirm whether this appearance aligns with the design system and is intentional.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/components/sidebar.svelte (1)
280-315: Small viewport feedback/support still exclude organizationsThe small-viewport condition is still:
{#if project && $isSmallViewport && !$isTabletViewport}So on organization pages (no
projectbut withorganizationId), Feedback/Support buttons remain hidden on small/mobile viewports, while the tablet block below correctly handles(project || organizationId).Given the PR objective that organization pages should show Feedback/Support on mobile/tablet, this is the same issue already flagged in a previous review and still needs adjustment.
To align small and tablet behavior:
-{#if project && $isSmallViewport && !$isTabletViewport} +{#if (project || organizationId) && $isSmallViewport && !$isTabletViewport}The analytics changes inside (renamed
'click_menu_feedback'event, wrapped label in<span>) and the removal of inline modal fragments look good and match the new top-level modal structure.
🧹 Nitpick comments (2)
src/lib/components/sidebar.svelte (2)
57-68:organizationIdprop wiring looks good; default assignment is redundantThe new
organizationIdprop is correctly added to$$Propsand exported with a compatible type. You can drop the explicit= undefinedinitializer for a tiny cleanup, since Svelte already defaultsexport letprops toundefinedwhen not provided.-export let organizationId: string | undefined = undefined; +export let organizationId: string | undefined;
257-279: Redundant nested{#if project}can be simplifiedInside an outer
{#if project}(starting at Line 172), you add another{#if project}wrapper around themobile-tablet-settingsblock. Given the outer guard, the inner condition is always true and just adds nesting noise.You can simplify by dropping the inner condition and keeping the contents:
- {#if project} - <div class="mobile-tablet-settings"> + <div class="mobile-tablet-settings"> <Tooltip placement="right" disabled={state !== 'icons'}> <a href={`${base}/project-${project.region}-${project.$id}/settings`} on:click={() => { trackEvent('click_menu_settings'); sideBarIsOpen = false; }} class="link" class:active={isSelected('/settings') && !isSelected('sites')} ><span class="link-icon"><Icon icon={IconCog} size="s" /></span ><span class:no-text={state === 'icons'} class:has-text={state === 'open'} class="link-text">Settings</span ></a> <span slot="tooltip">Settings</span> </Tooltip> - </div> - {/if} + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
src/lib/components/sidebar.svelte(10 hunks)
🔇 Additional comments (4)
src/lib/components/sidebar.svelte (4)
133-147: Organization upgrade CTA block matches org-only sidebar behaviorThe
!project && organizationIdguard, upgrade CTA, analytics eventClick.OrganizationClickUpgrade, andhref={${base}/organization-${organizationId}/change-plan}all align with showing an org-only upgrade entry in the sidebar. Behavior looks consistent with the project-level links above.
338-366: Tablet action buttons correctly include organization contextThis tablet-specific block now renders Feedback/Support when either a
projectororganizationIdis present:{#if (project || organizationId) && $isTabletViewport}That matches the requirement for organization pages to expose these actions on tablet viewports, and the event tracking/source metadata is consistent with the small-viewport block.
370-373: Confirm viewport gating for mobile feedback/support modalsThe
MobileFeedbackModalandMobileSupportModalare now rendered only under:{#if $isTabletViewport} <MobileFeedbackModal /> <MobileSupportModal bind:show={$showSupportModal} /> {/if}Given that the small-viewport DropList
slot="other"fragments are commented out, please double-check that$isTabletViewportinviewport.tsintentionally covers both mobile and tablet widths. If$isTabletViewportis truly “tablet-only” (e.g., 768–1023px) andisSmallViewportrepresents smaller widths, mobile might no longer render these modals from the sidebar.If that’s the case, consider broadening the condition:
{#if $isSmallViewport || $isTabletViewport} ... {/if}or otherwise ensure the mobile path renders the same modals from elsewhere.
400-408: Upgrade button styles are scoped and consistent
.upgrade-button-containerplus the nested:global(a)rule neatly ensures the upgrade anchor fills the container width and centers its text without impacting other sidebar links. The spacing matches surrounding components (margin-bottom: var(--space-6, 12px);).
2 participants
What does this PR do?
Problem
Organization Pages:
Project Pages:
Modal Issues:
Solution
For Organization Pages:
isOrganizationPagereactive variable in shell.svelte(isProjectPage || isOrganizationPage)organizationIdprop to Sidebar componentFor Project Pages:
$isTabletViewport(< 1024px)For Modal Functionality:
$isTabletViewportis trueChanges Made
File:
src/lib/layout/shell.svelteisOrganizationPagereactive variable(isProjectPage || isOrganizationPage)organizationId={$organization?.$id}prop to SidebarFile:
src/lib/components/sidebar.svelteorganizationIdprop to component interface$isTabletViewportFiles:
mobileFeedbackModal.svelte&mobileSupportModal.svelteTest Plan
Local Testing Performed:
pnpm lintandpnpm format- all checks passedScreenshots
Before Fix:
https://github.com/user-attachments/assets/4651a4ed-2da6-4299-bf09-31027fde44c9
After Fix:
https://github.com/user-attachments/assets/1a72c7b9-39fa-41cd-ae4c-002ef42e82cf
Related PRs and Issues
Fixes #2484
Have you read the Contributing Guidelines on issues?
Yes, I have read and followed the contributing guidelines.
Summary by CodeRabbit
New Features
Style