Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts PIN/biometric screens’ typography and layout to prevent oversized text and improve visual alignment in authentication flows.
Changes:
- PinScreen: replaces scroll-based layout with weighted/spacer-based vertical centering and sets explicit title/subtitle font size.
- BiometricScreen: applies a configurable font size to the “text above PIN” and tweaks subtitle typography style.
- BiometricViewModel: adds a
titleFontSizefield to state (currently not wired through to the UI).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/src/main/java/com/k689/identid/ui/common/pin/PinScreen.kt | Moves from verticalScroll to weighted layout/spacers and sets explicit text sizing/alignment. |
| app/src/main/java/com/k689/identid/ui/common/biometric/BiometricViewModel.kt | Adds titleFontSize to state (but currently unused). |
| app/src/main/java/com/k689/identid/ui/common/biometric/BiometricScreen.kt | Adds textFontSize parameter and uses it in text styles; changes subtitle typography. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Column( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxSize() | ||
| .padding(paddingValues) | ||
| .verticalScroll(rememberScrollState()), | ||
| .weight(1.0f), | ||
| ) { |
There was a problem hiding this comment.
Content no longer scrolls. If the available height is smaller than the combined height of the header texts + PIN field (e.g., small devices, landscape, or large accessibility font), the content can become clipped with no way to reach it. Consider restoring a verticalScroll(rememberScrollState()) (or using a LazyColumn) so the PIN entry remains reachable in constrained layouts.
| Column( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxSize() |
There was a problem hiding this comment.
This Column is both fillMaxSize() and weight(1f) as a direct child of the parent Column. Combining fillMaxSize with weight is redundant and can lead to confusing measurement behavior; typically you want fillMaxWidth() with weight(1f) (or drop weight if fillMaxSize is intended).
| .fillMaxSize() | |
| .fillMaxWidth() |
| PinFieldLayout( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| state = state, | ||
| onPinInput = { quickPin -> | ||
| onEventSend(Event.OnQuickPinEntered(quickPin)) | ||
| }, | ||
| ) | ||
| Spacer(modifier = Modifier.weight(2.8f)) | ||
| } |
There was a problem hiding this comment.
Spacer(modifier = Modifier.weight(2.8f)) introduces a hard-to-reason-about magic ratio in the layout. Consider replacing this with a named constant (documenting the intent) or using a more deterministic arrangement (e.g., verticalArrangement = Arrangement.SpaceBetween/Center) to avoid future layout regressions.
| data class State( | ||
| val isLoading: Boolean = false, | ||
| val error: ContentErrorConfig? = null, | ||
| val config: BiometricUiConfig, | ||
| val quickPinError: String? = null, | ||
| val quickPin: String = "", | ||
| val userBiometricsAreEnabled: Boolean = false, | ||
| val isBackable: Boolean = false, | ||
| val notifyOnAuthenticationFailure: Boolean = true, | ||
| val quickPinSize: Int = 6, | ||
| val isPinInputEnabled: Boolean = true, | ||
| val titleFontSize: TextUnit = 16.sp, | ||
| ) : ViewState |
There was a problem hiding this comment.
titleFontSize was added to State, but it’s not referenced anywhere (including BiometricScreen). As-is, this adds Compose UI types to the ViewModel state without effect. Either remove this field or wire it through by passing state.titleFontSize into the UI.
| private fun ColumnScope.MainContent( | ||
| state: State, | ||
| onEventSent: (event: Event) -> Unit, | ||
| textFontSize: TextUnit = 16.sp, | ||
| ) { |
There was a problem hiding this comment.
MainContent introduces textFontSize, but the only call site in this file (Body → MainContent(state, onEventSent)) doesn’t pass it, so it will always be the default. Either pass a value from state/config (e.g., state.titleFontSize) or remove this parameter to avoid dead customization points.
No description provided.