-
Notifications
You must be signed in to change notification settings - Fork 0
hw2 ver 1.0 #1
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
base: master
Are you sure you want to change the base?
hw2 ver 1.0 #1
Conversation
WordyArc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 + 3 + 0 + 0 = 7, принято
Что хорошо сделано:
Нормальный UiState sealed interface, Input/Result разделены
SplitCalculation считает derived-поля через get() - нормально, нет дублирования.
navigateToInput() действительно реализует “Back to edit” корректно(я уже успел повстречать работы где это было не так)
Навигация через Screen sealed class - плюс.
| onBackToEdit: () -> Unit, | ||
| onNewCalculation: () -> Unit | ||
| ) { | ||
| val calculations by viewModel.calculations.collectAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Для чего это?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Убрать "мертвый код"
| onBackToEdit() | ||
| }, | ||
| onNewCalculationClicked = { | ||
| viewModel.startNewCalculation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
тут и в SplitMateNavigation в onNewCalculation двойной вызов startNewCalculation.
Или “UI вызывает VM и говорит навигации куда идти”
Или “навигация дергает VM”
Но не оба одновременно.
| val calculations by viewModel.calculations.collectAsState() | ||
| val uiState by viewModel.uiState.collectAsState() | ||
|
|
||
| LaunchedEffect(calculationId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Очень странная логика.
если calculation == null - вы вообще ничего не показываете.
when/else ветки пустые (else -> {}) - так делать нельзя.
Если calcId не найден - показать UI “Расчёт не найден” + кнопка “На главный/к вводу”.
Упростить: ResultScreen должен опираться на calcId и доставать calculation напрямую, без плясок вокруг uiState. uiState тут вообще лишний - маршрут уже говорит, что мы на Result.
| value = state.totalAmount, | ||
| onValueChange = onTotalAmountChanged, | ||
| label = { Text("Полная сумма счета (₽)") }, | ||
| keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totalAmount - Double
No description provided.