Add dark mode display settings and settings access from login#256
Open
AndrewCheung360 wants to merge 4 commits intomainfrom
Open
Add dark mode display settings and settings access from login#256AndrewCheung360 wants to merge 4 commits intomainfrom
AndrewCheung360 wants to merge 4 commits intomainfrom
Conversation
…ogin - Add DisplayBottomSheet with Light / Dark / Device Theme radio options - Add Display row to SettingsScreen wired to ThemeViewModel - Expose isLoggedIn from ThemeViewModel; hide logout button when not logged in - Make settings list scrollable to prevent logout button from squishing - Add settings icon to LoginPage header so unauthenticated users can access settings - Add settings_display_* string resources Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a theme selection flow (light/dark/system) and wiring: new ThemePreference model and flows, ThemeViewModel changes, DisplayBottomSheet UI, SettingsScreen integration, LoginPage settings button, and related string resources and repo mapping. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SettingsScreen as SettingsScreen
participant DisplayBottomSheet as DisplayBottomSheet
participant ThemeViewModel as ThemeViewModel
participant Repository as UserPreferencesRepository
User->>SettingsScreen: Tap "Display" option
SettingsScreen->>SettingsScreen: showDisplaySheet = true
SettingsScreen->>DisplayBottomSheet: open(themePreference)
User->>DisplayBottomSheet: Selects Light/Dark/System
DisplayBottomSheet->>DisplayBottomSheet: update selected state
User->>DisplayBottomSheet: Tap Done
DisplayBottomSheet->>ThemeViewModel: call onLight/onDark/onSystem
ThemeViewModel->>Repository: update persisted preference
DisplayBottomSheet->>SettingsScreen: onDismiss
SettingsScreen->>SettingsScreen: showDisplaySheet = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/android/eatery/ui/screens/SettingsScreen.kt`:
- Around line 365-367: The Logout icon currently uses
Icons.AutoMirrored.Filled.Logout.name as its contentDescription which exposes
non-localized, noisy text to accessibility services; update the Icon usage
associated with the logout action (the Icon instantiation referencing
Icons.AutoMirrored.Filled.Logout and contentDescription) to use
contentDescription = null (or a localized stringResource if you intend a spoken
label), so TalkBack will not read the redundant icon label next to the visible
"Log out" text.
- Line 32: Replace the direct Flow collection call using collectAsState() in
SettingsScreen (where the ViewModel/state flows are converted to Compose state)
with the lifecycle-aware collectAsStateWithLifecycle() to match the module
pattern; update the usages that reference the ViewModel's flows (e.g.,
settingsViewModel.someStateFlow -> collectAsStateWithLifecycle()) so the
composable observes state with the lifecycle-aware API (ensure you import
collectAsStateWithLifecycle and adjust any nullable/default parameters
accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a56e8de-539d-4522-b11a-0a357519eeb0
📒 Files selected for processing (6)
app/src/main/java/com/cornellappdev/android/eatery/ui/components/login/LoginPage.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/components/settings/DisplayBottomSheet.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/ProfileScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/screens/SettingsScreen.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/viewmodels/ThemeViewModel.ktapp/src/main/res/values/strings.xml
- Use collectAsStateWithLifecycle() instead of collectAsState() for lifecycle-aware state collection - Set logout icon contentDescription to null to avoid TalkBack reading redundant icon label alongside visible "Log out" text Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
caleb-bit
requested changes
Apr 28, 2026
Contributor
caleb-bit
left a comment
caleb-bit
left a comment
There was a problem hiding this comment.
I think UI looks good, but we should probably review the way that we're handling how the color scheme state is being represented (currently a Boolean?).
| initialValue = null | ||
| ) | ||
|
|
||
| val isLoggedIn: StateFlow<Boolean> = repository.isLoggedInFlow.stateIn( |
Contributor
There was a problem hiding this comment.
Does this state belong here? Why not have this in SettingsViewModel?
Member
Author
There was a problem hiding this comment.
good point, should only be used for determining whether the logout button should appear in settings
| private val repository: UserPreferencesRepository): ViewModel() | ||
| { | ||
| val isDarkMode : StateFlow<Boolean?> = repository.isDarkModeFlow.stateIn( | ||
| val isDarkMode: StateFlow<Boolean?> = repository.isDarkModeFlow.stateIn( |
Contributor
There was a problem hiding this comment.
I think it would be clearer semantically to deal with enums rather than nullable booleans. Keeping optional bool as the way the field is stored in protobufs, maybe UserPreferencesRepository should expose the state as an enum instead of handling that down in the UI in DisplayBottomSheet.
| <string name="settings_display_description">Choose a light or dark theme</string> | ||
| <string name="settings_display_light">Light</string> | ||
| <string name="settings_display_dark">Dark</string> | ||
| <string name="settings_display_system">Device Theme</string> |
Contributor
There was a problem hiding this comment.
Nit: Maybe just Device for consistency. Figma doesn't seem to specify, so it shouldn't matter too much. How does it look on iOS?
Member
Author
There was a problem hiding this comment.
I think it's the same on iOS, but we can change.
| <string name="settings_notifications_title">Notifications</string> | ||
| <string name="settings_notifications_description">Manage item and promotional notifications</string> | ||
| <string name="settings_display_title">Display</string> | ||
| <string name="settings_display_description">Choose a light or dark theme</string> |
Contributor
There was a problem hiding this comment.
Maybe "Choose color scheme"？ The wording seems a little awkward here. Is it the same on iOS?
Member
Author
There was a problem hiding this comment.
I think it's the same on iOS, but we can change.
…ingsViewModel - Add ThemePreference enum (LIGHT/DARK/SYSTEM) to data/models - Expose themePreferenceFlow from UserPreferencesRepository instead of nullable Boolean - Replace isDarkMode: StateFlow<Boolean?> with themePreference: StateFlow<ThemePreference> in ThemeViewModel; drop isLoggedIn - Add isLoggedIn: StateFlow<Boolean> to SettingsViewModel (injecting UserPreferencesRepository) - Update rememberResolvedDarkMode in Theme.kt to switch on ThemePreference enum - Update DisplayBottomSheet to accept ThemePreference param directly, removing private DisplayTheme enum - Update SettingsScreen to use ThemePreference and collect isLoggedIn from settingsViewModel Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds a Display settings option so users can choose Light, Dark, or Device Theme for the app's appearance. Also adds a settings gear icon to the login page so unauthenticated users can reach Settings without logging in first. Added spacing in upcoming menus between filter row and content.
Changes Made
New file —
DisplayBottomSheet.ktcurrentColorsandEateryBlueTypography; supports light and dark preview via@DualModePreviewSettingsScreen.ktThemeViewModelto collectisDarkModeandisLoggedInstateDisplayBottomSheetColumnso all items remain accessible on smaller screensThemeViewModel.ktisLoggedIn: StateFlow<Boolean>fromUserPreferencesRepository.isLoggedInFlowLoginPage.ktonSettingsClickedparameter (default no-op for backward compatibility)system_settingdrawable) added to the top-right of the nav row viaArrangement.SpaceBetweenProfileScreen.ktonSettingsClickedthrough toLoginPageso navigation wires up correctlyUpcomingMenusScreen.ktstrings.xmlsettings_display_title,settings_display_description,settings_display_light,settings_display_dark,settings_display_systemTest Coverage
Next Steps (optional)
TODO
Related PRs or Issues (optional)
Screenshots (optional)
Summary by CodeRabbit
New Features
Improvements
Other