refactor generic CountryCodeBottomSheet and MessengersBottomSheet fo…#2
Open
yibibyte wants to merge 1 commit intocompose-startfrom
Open
refactor generic CountryCodeBottomSheet and MessengersBottomSheet fo…#2yibibyte wants to merge 1 commit intocompose-startfrom
yibibyte wants to merge 1 commit intocompose-startfrom
Conversation
… CommonBottomSheet
Руководство для ревьюераРефакторинг нижних листов (bottom sheets) с выбором кода страны и мессенджеров: логика разметки и обработки выбора делегирована общему компоненту Диаграмма последовательностей обработки выбора через CommonBottomSheetsequenceDiagram
actor User
participant MessengersBottomSheet
participant CommonBottomSheet
participant MessengerOption
User->>MessengersBottomSheet: open filter_by_messaging_app
MessengersBottomSheet->>CommonBottomSheet: render(title, items, selectedItems, onItemsSelected, onDismiss, itemContent)
loop for each MessagingApp
CommonBottomSheet->>MessengerOption: render(app, isSelected)
end
User->>CommonBottomSheet: tap app row or checkbox
CommonBottomSheet->>CommonBottomSheet: compute newSelection
CommonBottomSheet-->>MessengersBottomSheet: onItemsSelected(newSelection)
MessengersBottomSheet-->>User: sheet shows updated selection state
Изменения на уровне файлов
Подсказки и командыВзаимодействие с Sourcery
Настройка работыЗайдите в свою панель управления, чтобы:
Получение помощи
Original review guide in EnglishReviewer's GuideRefactors the country code and messengers filter bottom sheets to delegate layout and selection handling to a shared CommonBottomSheet component, simplifying the option composables to be purely presentational. Sequence diagram for selection handling via CommonBottomSheetsequenceDiagram
actor User
participant MessengersBottomSheet
participant CommonBottomSheet
participant MessengerOption
User->>MessengersBottomSheet: open filter_by_messaging_app
MessengersBottomSheet->>CommonBottomSheet: render(title, items, selectedItems, onItemsSelected, onDismiss, itemContent)
loop for each MessagingApp
CommonBottomSheet->>MessengerOption: render(app, isSelected)
end
User->>CommonBottomSheet: tap app row or checkbox
CommonBottomSheet->>CommonBottomSheet: compute newSelection
CommonBottomSheet-->>MessengersBottomSheet: onItemsSelected(newSelection)
MessengersBottomSheet-->>User: sheet shows updated selection state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Привет! Я посмотрел твои изменения — вот немного обратной связи:
- Теперь, когда
CheckboxиспользуетonCheckedChange = nullи полагается на обработчик клика родительского элемента, стоит добавить подходящийModifier.selectable/семантику к строке (или снова включить клик по Checkbox), чтобы сохранить доступность и предсказуемость для пользователей, которые ожидают нажимать непосредственно на чекбокс.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `Checkbox` uses `onCheckedChange = null` and relies on the parent click handler, consider adding appropriate `Modifier.selectable`/semantics to the row (or re-enabling Checkbox click) to preserve accessibility and discoverability for users who expect to tap directly on the checkbox.
## Individual Comments
### Comment 1
<location> `app/src/main/java/ru/yandex/practicum/contacts/presentation/messengers/MessengersBottomSheet.kt:50-52` </location>
<code_context>
) {
Checkbox(
checked = isSelected,
- onCheckedChange = { checked ->
- val newSelection = selectedCodes.toMutableSet()
- if (checked) {
- newSelection.add(countryCode)
- } else {
- newSelection.remove(countryCode)
- }
- onCodesSelected(newSelection)
- }
+ onCheckedChange = null
)
Spacer(modifier = Modifier.width(16.dp))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wire the checkbox to the same selection handler used by the parent for better accessibility and UX.
The checkbox appears selected but is not directly interactive (`onCheckedChange = null`), so only the parent row in `CommonBottomSheet` toggles the state. This can confuse users and assistive technologies that expect the checkbox itself to handle taps. Please pass the same toggle lambda used in `CommonBottomSheet` down to `MessengerOption` and connect it to `onCheckedChange`, or alternatively make the row `Modifier.toggleable` with `role = Role.Checkbox` so accessibility tools correctly recognize and activate it.
Suggested implementation:
```
private fun MessengerOption(
isSelected: Boolean,
app: MessagingApp,
onCheckedChange: (Boolean) -> Unit
) {
Row(
modifier = Modifier
) {
Checkbox(
checked = isSelected,
onCheckedChange = onCheckedChange
)
Spacer(modifier = Modifier.width(16.dp))
Text(app.name)
```
To fully wire this up, you will also need to:
1. Update all call sites of `MessengerOption` in `MessengersBottomSheet.kt` to pass the existing selection handler used by `CommonBottomSheet` for row taps. For example:
- Before: `MessengerOption(isSelected = isSelected, app = app)`
- After: `MessengerOption(isSelected = isSelected, app = app, onCheckedChange = { checked -> /* existing toggle logic for this app, e.g. toggleMessenger(app, checked) */ })`
2. Ensure that the lambda passed to `onCheckedChange` updates the same selection state that the parent row currently toggles, so both tapping the row and the checkbox keep the UI in sync and accessible.
</issue_to_address>
### Comment 2
<location> `app/src/main/java/ru/yandex/practicum/contacts/presentation/country/CountryCodeBottomSheet.kt:52-54` </location>
<code_context>
) {
Checkbox(
checked = isSelected,
- onCheckedChange = { checked ->
- val newSelection = selectedCodes.toMutableSet()
- if (checked) {
- newSelection.add(countryCode)
- } else {
- newSelection.remove(countryCode)
- }
- onCodesSelected(newSelection)
- }
+ onCheckedChange = null
)
Spacer(modifier = Modifier.width(16.dp))
</code_context>
<issue_to_address>
**suggestion:** Avoid a non-interactive checkbox by delegating the selection toggle into this composable.
If `CommonBottomSheet` owns the selection, consider exposing an `onToggle` lambda from the item content slot and passing it through as `onCheckedChange` here. That way the row and checkbox both trigger the same state change and the checkbox remains interactive and accessible rather than being display-only.
Suggested implementation:
```
private fun CountryCodeOption(
isSelected: Boolean,
countryCode: CountryCode,
onToggle: () -> Unit,
) {
Row(
modifier = Modifier
.clickable(onClick = onToggle)
) {
Checkbox(
checked = isSelected,
onCheckedChange = { onToggle() }
)
Spacer(modifier = Modifier.width(16.dp))
Column {
```
1. Ensure `clickable` is imported at the top of the file:
- `import androidx.compose.foundation.clickable`
2. Update all call sites of `CountryCodeOption` to pass an `onToggle` lambda that delegates to the state owner. For example, where you build the list items in `CommonBottomSheet` (or the relevant composable), change:
- `CountryCodeOption(isSelected = isSelected, countryCode = countryCode)`
- to something like:
- `CountryCodeOption(isSelected = isSelected, countryCode = countryCode, onToggle = { onCodesSelected(/* updated selection */) })`
3. If you’re using a content lambda for `CommonBottomSheet` items, extend that slot to provide `onToggle: () -> Unit` to the item content so it can be forwarded into `CountryCodeOption`.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey there - I've reviewed your changes - here's some feedback:
- Now that
CheckboxusesonCheckedChange = nulland relies on the parent click handler, consider adding appropriateModifier.selectable/semantics to the row (or re-enabling Checkbox click) to preserve accessibility and discoverability for users who expect to tap directly on the checkbox.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `Checkbox` uses `onCheckedChange = null` and relies on the parent click handler, consider adding appropriate `Modifier.selectable`/semantics to the row (or re-enabling Checkbox click) to preserve accessibility and discoverability for users who expect to tap directly on the checkbox.
## Individual Comments
### Comment 1
<location> `app/src/main/java/ru/yandex/practicum/contacts/presentation/messengers/MessengersBottomSheet.kt:50-52` </location>
<code_context>
) {
Checkbox(
checked = isSelected,
- onCheckedChange = { checked ->
- val newSelection = selectedCodes.toMutableSet()
- if (checked) {
- newSelection.add(countryCode)
- } else {
- newSelection.remove(countryCode)
- }
- onCodesSelected(newSelection)
- }
+ onCheckedChange = null
)
Spacer(modifier = Modifier.width(16.dp))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wire the checkbox to the same selection handler used by the parent for better accessibility and UX.
The checkbox appears selected but is not directly interactive (`onCheckedChange = null`), so only the parent row in `CommonBottomSheet` toggles the state. This can confuse users and assistive technologies that expect the checkbox itself to handle taps. Please pass the same toggle lambda used in `CommonBottomSheet` down to `MessengerOption` and connect it to `onCheckedChange`, or alternatively make the row `Modifier.toggleable` with `role = Role.Checkbox` so accessibility tools correctly recognize and activate it.
Suggested implementation:
```
private fun MessengerOption(
isSelected: Boolean,
app: MessagingApp,
onCheckedChange: (Boolean) -> Unit
) {
Row(
modifier = Modifier
) {
Checkbox(
checked = isSelected,
onCheckedChange = onCheckedChange
)
Spacer(modifier = Modifier.width(16.dp))
Text(app.name)
```
To fully wire this up, you will also need to:
1. Update all call sites of `MessengerOption` in `MessengersBottomSheet.kt` to pass the existing selection handler used by `CommonBottomSheet` for row taps. For example:
- Before: `MessengerOption(isSelected = isSelected, app = app)`
- After: `MessengerOption(isSelected = isSelected, app = app, onCheckedChange = { checked -> /* existing toggle logic for this app, e.g. toggleMessenger(app, checked) */ })`
2. Ensure that the lambda passed to `onCheckedChange` updates the same selection state that the parent row currently toggles, so both tapping the row and the checkbox keep the UI in sync and accessible.
</issue_to_address>
### Comment 2
<location> `app/src/main/java/ru/yandex/practicum/contacts/presentation/country/CountryCodeBottomSheet.kt:52-54` </location>
<code_context>
) {
Checkbox(
checked = isSelected,
- onCheckedChange = { checked ->
- val newSelection = selectedCodes.toMutableSet()
- if (checked) {
- newSelection.add(countryCode)
- } else {
- newSelection.remove(countryCode)
- }
- onCodesSelected(newSelection)
- }
+ onCheckedChange = null
)
Spacer(modifier = Modifier.width(16.dp))
</code_context>
<issue_to_address>
**suggestion:** Avoid a non-interactive checkbox by delegating the selection toggle into this composable.
If `CommonBottomSheet` owns the selection, consider exposing an `onToggle` lambda from the item content slot and passing it through as `onCheckedChange` here. That way the row and checkbox both trigger the same state change and the checkbox remains interactive and accessible rather than being display-only.
Suggested implementation:
```
private fun CountryCodeOption(
isSelected: Boolean,
countryCode: CountryCode,
onToggle: () -> Unit,
) {
Row(
modifier = Modifier
.clickable(onClick = onToggle)
) {
Checkbox(
checked = isSelected,
onCheckedChange = { onToggle() }
)
Spacer(modifier = Modifier.width(16.dp))
Column {
```
1. Ensure `clickable` is imported at the top of the file:
- `import androidx.compose.foundation.clickable`
2. Update all call sites of `CountryCodeOption` to pass an `onToggle` lambda that delegates to the state owner. For example, where you build the list items in `CommonBottomSheet` (or the relevant composable), change:
- `CountryCodeOption(isSelected = isSelected, countryCode = countryCode)`
- to something like:
- `CountryCodeOption(isSelected = isSelected, countryCode = countryCode, onToggle = { onCodesSelected(/* updated selection */) })`
3. If you’re using a content lambda for `CommonBottomSheet` items, extend that slot to provide `onToggle: () -> Unit` to the item content so it can be forwarded into `CountryCodeOption`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
50
to
+52
| Checkbox( | ||
| checked = isSelected, | ||
| onCheckedChange = { checked -> | ||
| val newSelection = selectedApps.toMutableSet() | ||
| if (checked) { | ||
| newSelection.add(app) | ||
| } else { | ||
| newSelection.remove(app) | ||
| } | ||
| onAppsSelected(newSelection) | ||
| } | ||
| onCheckedChange = null |
There was a problem hiding this comment.
suggestion (bug_risk): Подключите чекбокс к тому же обработчику выбора, который используется родительским элементом, чтобы улучшить доступность и UX.
Сейчас чекбокс выглядит выбранным, но напрямую не интерактивен (onCheckedChange = null), поэтому состояние переключает только родительская строка в CommonBottomSheet. Это может сбивать с толку пользователей и вспомогательные технологии, которые ожидают, что нажатия будет обрабатывать сам чекбокс. Пожалуйста, передайте ту же лямбду-переключатель, что используется в CommonBottomSheet, вниз в MessengerOption и свяжите её с onCheckedChange, либо сделайте строку Modifier.toggleable с role = Role.Checkbox, чтобы инструменты доступности корректно распознавали и активировали её.
Предлагаемая реализация:
private fun MessengerOption(
isSelected: Boolean,
app: MessagingApp,
onCheckedChange: (Boolean) -> Unit
) {
Row(
modifier = Modifier
) {
Checkbox(
checked = isSelected,
onCheckedChange = onCheckedChange
)
Spacer(modifier = Modifier.width(16.dp))
Text(app.name)
Чтобы полностью всё связать, также нужно:
- Обновить все вызовы
MessengerOptionвMessengersBottomSheet.kt, чтобы они передавали существующий обработчик выбора, который сейчас используетCommonBottomSheetдля нажатий по строке. Например:- До:
MessengerOption(isSelected = isSelected, app = app) - После:
MessengerOption(isSelected = isSelected, app = app, onCheckedChange = { checked -> /* существующая логика переключения для этого приложения, например toggleMessenger(app, checked) */ })
- До:
- Убедиться, что лямбда, передаваемая в
onCheckedChange, обновляет то же состояние выбора, которое сейчас переключает родительская строка, чтобы и нажатие по строке, и по чекбоксу держали UI синхронизированным и доступным.
Original comment in English
suggestion (bug_risk): Wire the checkbox to the same selection handler used by the parent for better accessibility and UX.
The checkbox appears selected but is not directly interactive (onCheckedChange = null), so only the parent row in CommonBottomSheet toggles the state. This can confuse users and assistive technologies that expect the checkbox itself to handle taps. Please pass the same toggle lambda used in CommonBottomSheet down to MessengerOption and connect it to onCheckedChange, or alternatively make the row Modifier.toggleable with role = Role.Checkbox so accessibility tools correctly recognize and activate it.
Suggested implementation:
private fun MessengerOption(
isSelected: Boolean,
app: MessagingApp,
onCheckedChange: (Boolean) -> Unit
) {
Row(
modifier = Modifier
) {
Checkbox(
checked = isSelected,
onCheckedChange = onCheckedChange
)
Spacer(modifier = Modifier.width(16.dp))
Text(app.name)
To fully wire this up, you will also need to:
- Update all call sites of
MessengerOptioninMessengersBottomSheet.ktto pass the existing selection handler used byCommonBottomSheetfor row taps. For example:- Before:
MessengerOption(isSelected = isSelected, app = app) - After:
MessengerOption(isSelected = isSelected, app = app, onCheckedChange = { checked -> /* existing toggle logic for this app, e.g. toggleMessenger(app, checked) */ })
- Before:
- Ensure that the lambda passed to
onCheckedChangeupdates the same selection state that the parent row currently toggles, so both tapping the row and the checkbox keep the UI in sync and accessible.
Comment on lines
52
to
+54
| Checkbox( | ||
| checked = isSelected, | ||
| onCheckedChange = { checked -> | ||
| val newSelection = selectedCodes.toMutableSet() | ||
| if (checked) { | ||
| newSelection.add(countryCode) | ||
| } else { | ||
| newSelection.remove(countryCode) | ||
| } | ||
| onCodesSelected(newSelection) | ||
| } | ||
| onCheckedChange = null |
There was a problem hiding this comment.
suggestion: Избегайте неинтерактивного чекбокса, делегируя переключение выбора в этот composable.
Если выбор контролируется CommonBottomSheet, рассмотрите возможность пробросить наружу лямбду onToggle из слота контента элемента и передать её сюда как onCheckedChange. Так и строка, и чекбокс будут вызывать одно и то же изменение состояния, а чекбокс останется интерактивным и доступным, а не только отображаемым.
Предлагаемая реализация:
private fun CountryCodeOption(
isSelected: Boolean,
countryCode: CountryCode,
onToggle: () -> Unit,
) {
Row(
modifier = Modifier
.clickable(onClick = onToggle)
) {
Checkbox(
checked = isSelected,
onCheckedChange = { onToggle() }
)
Spacer(modifier = Modifier.width(16.dp))
Column {
- Убедитесь, что
clickableимпортирован вверху файла:import androidx.compose.foundation.clickable
- Обновите все вызовы
CountryCodeOption, чтобы они передавали лямбдуonToggle, делегирующую управление владельцу состояния. Например, там, где вы строите элементы списка вCommonBottomSheet(или соответствующем composable), измените:CountryCodeOption(isSelected = isSelected, countryCode = countryCode)- на что-то вроде:
CountryCodeOption(isSelected = isSelected, countryCode = countryCode, onToggle = { onCodesSelected(/* updated selection */) })
- Если вы используете лямбду контента для элементов
CommonBottomSheet, расширьте этот слот так, чтобы он предоставлялonToggle: () -> Unitв контент элемента и его можно было пробросить дальше вCountryCodeOption.
Original comment in English
suggestion: Avoid a non-interactive checkbox by delegating the selection toggle into this composable.
If CommonBottomSheet owns the selection, consider exposing an onToggle lambda from the item content slot and passing it through as onCheckedChange here. That way the row and checkbox both trigger the same state change and the checkbox remains interactive and accessible rather than being display-only.
Suggested implementation:
private fun CountryCodeOption(
isSelected: Boolean,
countryCode: CountryCode,
onToggle: () -> Unit,
) {
Row(
modifier = Modifier
.clickable(onClick = onToggle)
) {
Checkbox(
checked = isSelected,
onCheckedChange = { onToggle() }
)
Spacer(modifier = Modifier.width(16.dp))
Column {
- Ensure
clickableis imported at the top of the file:import androidx.compose.foundation.clickable
- Update all call sites of
CountryCodeOptionto pass anonTogglelambda that delegates to the state owner. For example, where you build the list items inCommonBottomSheet(or the relevant composable), change:CountryCodeOption(isSelected = isSelected, countryCode = countryCode)- to something like:
CountryCodeOption(isSelected = isSelected, countryCode = countryCode, onToggle = { onCodesSelected(/* updated selection */) })
- If you’re using a content lambda for
CommonBottomSheetitems, extend that slot to provideonToggle: () -> Unitto the item content so it can be forwarded intoCountryCodeOption.
interactivetextbook
left a comment
interactivetextbook
left a comment
There was a problem hiding this comment.
Хорошая работа, молодец!
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
3 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.
Refactor CountryCodeBottomSheet and MessengersBottomSheet for CommonBottomSheet
Summary by Sourcery
Рефакторинг нижних листов выбора кода страны и мессенджера с использованием общего универсального компонента
CommonBottomSheet.Улучшения:
ModalBottomSheetв фильтрах кода страны и мессенджеров на повторно используемый компонентCommonBottomSheet.CountryCodeOptionиMessengerOption, сделав их статeless и делегировав обработку выбора общей логике нижнего листа.Original summary in English
Summary by Sourcery
Refactor country code and messenger selection bottom sheets to use a shared generic CommonBottomSheet component.
Enhancements: