Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: SettingsFragment to compose #1516

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

materoy
Copy link

@materoy materoy commented Jan 1, 2025

This pr is a refactor of SettingsFragment to SettingsScreen in compose

Still in draft phase, almost complete, feel free to review, suggest edits and bug fixes

@CLAassistant
Copy link

CLAassistant commented Jan 1, 2025

CLA assistant check
All committers have signed the CLA.

@materoy materoy marked this pull request as draft January 1, 2025 22:02
@materoy materoy changed the title Settings screen refactor to compose refactor: SettingsFragment to compose Jan 1, 2025
@andrekir
Copy link
Member

andrekir commented Jan 3, 2025

Thanks for your contribution! As a general guideline, we want to follow the design of a traditional Settings/Preference screen, making use of our existing "Preference" components (like SwitchPreference and DropdownPreference).

The BLE/TCP/Serial device scan/selection could probably move to an item in the Settings screen to maintain a cleaner layout, but open to ideas and suggestions on how best to organize this.

Items currently in the overflow menu, like Theme and Language, will also be moved into this screen in the future.

devices.value = mutableMapOf<String, DeviceListEntry>().apply {
fun addDevice(entry: DeviceListEntry) { this[entry.fullAddress] = entry }
suspend fun addDevice(entry: DeviceListEntry) {
_uiState.emit(uiState.value.copy(devices = uiState.value.devices + (entry.fullAddress to entry)))
Copy link
Member

Choose a reason for hiding this comment

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

use MutableStateFlow .update {} instead of .emit

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.

3 participants