Skip to content

feat/pane-expansion : Add expansion anchors and enforce 250dp minimum…#81

Open
prateekbatra-g wants to merge 2 commits into
mainfrom
feat/pane-expansion-update
Open

feat/pane-expansion : Add expansion anchors and enforce 250dp minimum…#81
prateekbatra-g wants to merge 2 commits into
mainfrom
feat/pane-expansion-update

Conversation

@prateekbatra-g

Copy link
Copy Markdown
Contributor

Improves the adaptive Master-Detail layout (CahierNavigationSuite) resizing behavior on large-screen devices. It adds snap-to anchors and guarantees that neither the list nor the detail pane is squished past legibility during resizing.
Key Changes

  • Pane Expansion Anchors: Added snap positions in rememberPaneExpansionState at 0% (collapsed), 25% (minimum snap), 50% (half split), and 100% (expanded).
  • 250dp Minimum Width: Enforced a 250.dp (MinPaneWidth) constraint using a custom .layout modifier on both listPane and detailPane.
  • Refactoring: Extracted MinPaneWidth as a clean, file-level constant to prevent magic numbers and keep Composable signatures lightweight.
Before After
How Cahier Currently do After Snap Pointers Implementations

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request configures pane expansion anchors for the list-detail scaffold, applies a custom layout modifier to enforce a minimum width on both the list and detail panes, and adjusts z-indices and elevations for proper layering. The reviewer suggests extracting the duplicated layout logic into a reusable Modifier extension function that handles clipping and collapsed states to prevent visual bleeding. Additionally, the reviewer highlights a discrepancy between the defined snapping anchors and the initial pane proportion of 0.3f, which could cause a jumpy user experience.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

import kotlinx.coroutines.launch


private val MinPaneWidth = 250.dp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

To avoid duplicating the complex .layout modifier logic in both listPane and detailPane, and to fix potential layout bugs, we should extract it into a reusable Modifier extension function.

Key Improvements:

  1. Avoid Duplication: Keeps the code DRY and maintainable.
  2. Prevent Overlap/Bleeding: When a pane's allocated width is smaller than MinPaneWidth (e.g., during resizing or when collapsing), the child would normally draw outside its bounds and overlap adjacent panes. Applying clipToBounds() prevents this visual bleeding.
  3. Handle Collapsed State: If constraints.maxWidth == 0 (fully collapsed), we should avoid measuring and drawing the child at MinPaneWidth to prevent it from overlapping other visible content.
private val MinPaneWidth = 250.dp

private fun Modifier.minimumWidthLayout(minWidth: androidx.compose.ui.unit.Dp): Modifier = this
    .clipToBounds()
    .layout { measurable, constraints ->
        if (constraints.maxWidth == 0) {
            layout(0, 0) {}
        } else {
            val fixedWidth = minWidth.roundToPx()
            val placeable = measurable.measure(
                constraints.copy(
                    minWidth = fixedWidth,
                    maxWidth = maxOf(fixedWidth, constraints.maxWidth)
                )
            )
            val reportedWidth = constraints.maxWidth
            layout(reportedWidth, placeable.height) {
                placeable.placeRelative(0, 0)
            }
        }
    }

Comment on lines +276 to +293
modifier = Modifier
.fillMaxSize()
// The layout modifier implements a minimum width for the pane while
// allowing it to report its original width to the scaffold, preventing
// content from being overly compressed during resizing.
.layout { measurable, constraints ->
val fixedWidth = MinPaneWidth.roundToPx()
val placeable = measurable.measure(
constraints.copy(
minWidth = fixedWidth,
maxWidth = maxOf(fixedWidth, constraints.maxWidth)
)
)
val reportedWidth = constraints.maxWidth
layout(reportedWidth, placeable.height) {
placeable.placeRelative(0, 0)
}
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use the extracted minimumWidthLayout modifier to simplify the code and ensure correct clipping and collapsed state handling.

                                modifier = Modifier
                                    .fillMaxSize()
                                    .minimumWidthLayout(MinPaneWidth),

Comment on lines +335 to +355
modifier = Modifier
.fillMaxSize()
// Specify z-index to ensure the detail pane content is correctly layered
.zIndex(1f)
// The layout modifier implements a minimum width for the pane while
// allowing it to report its original width to the scaffold, preventing
// content from being overly compressed during resizing.
.layout { measurable, constraints ->
val fixedWidth = MinPaneWidth.roundToPx()
val placeable = measurable.measure(
constraints.copy(
minWidth = fixedWidth,
maxWidth = maxOf(fixedWidth, constraints.maxWidth)
)
)
val reportedWidth = constraints.maxWidth
layout(reportedWidth, placeable.height) {
// Place at 0 to maintain left-alignment within its allocated space
placeable.placeRelative(0, 0)
}
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use the extracted minimumWidthLayout modifier here as well to keep the implementation consistent and clean.

                                        modifier = Modifier
                                            .fillMaxSize()
                                            .zIndex(1f)
                                            .minimumWidthLayout(MinPaneWidth),

import androidx.compose.runtime.saveable.rememberSaveable
import androidx.compose.runtime.setValue
import androidx.compose.ui.Modifier
import androidx.compose.ui.layout.layout

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add the import for clipToBounds to support the custom minimum width layout modifier.

Suggested change
import androidx.compose.ui.layout.layout
import androidx.compose.ui.draw.clipToBounds
import androidx.compose.ui.layout.layout

Comment on lines +123 to +138
val paneExpansionState = rememberPaneExpansionState(
keyProvider = navigator.scaffoldValue,
anchors = listOf(
// 1. Fully collapsed state (0% screen fraction)
PaneExpansionAnchor.Proportion(proportion = 0f),

// 2. The 25 % minimum width snapping boundary anchor
PaneExpansionAnchor.Proportion(proportion = 0.25f),

// 3. Middle split state (50% screen fraction)
PaneExpansionAnchor.Proportion(proportion = 0.5f),

// 4. Fully expanded list state (100% screen fraction, detail collapsed)
PaneExpansionAnchor.Proportion(proportion = 1f)
)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a discrepancy between the defined anchors (0f, 0.25f, 0.5f, 1f) and the initial pane proportion of 0.3f set in the LaunchedEffect (line 175).

Because 0.3f is not one of the snapping anchors, the pane might immediately snap to 0.25f or 0.5f as soon as the user starts dragging the handle, leading to a jumpy user experience. Consider aligning the initial proportion with one of the defined anchors (e.g., 0.25f or 0.5f), or adding 0.3f as an anchor if it is a desired snap point.

@prateekbatra-g prateekbatra-g force-pushed the feat/pane-expansion-update branch from f01dc21 to c6281c2 Compare June 15, 2026 05:58

@cka-dev cka-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice update.

Only one thing: can you run the screenshot tests and make sure they are passing.
I think the change is affecting some of the existing screenshot tests.

@prateekbatra-g prateekbatra-g force-pushed the feat/pane-expansion-update branch from c6281c2 to 6e13e7e Compare June 18, 2026 05:40
@prateekbatra-g prateekbatra-g requested a review from cka-dev June 18, 2026 05:48

@cka-dev cka-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants