Enable/disable submit button on checkbox click#13957
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Thanks @zwarm! I've tested the app and it works as expected. I've left some suggestions, but they are not blocking this PR.
P.S. The changes are not unit tested - I know that the whole class is not unit tested yet, but adding at least tests for the introduced logic might be worth it. Wdyt?
| } | ||
| } | ||
| _uiState.postValue(details.copy(items = updatedList)) | ||
| _uiState.value?.let { |
There was a problem hiding this comment.
💡 Idea: We might want to consider using (_uiState.value as? DetailsState)?.let {...} instead of force casting the state in _uiState.postValue((it as DetailsState).copy(items = updatedItems)). Wdyt?
| uiState: BackupDownloadUiState, | ||
| itemType: JetpackAvailableItemType | ||
| ): List<JetpackListItemState> { | ||
| var checkedCount = 0 |
There was a problem hiding this comment.
💡 This code took me a while to understand. We might want to consider separating the "change state" logic and "count" logic. Perhaps something like this might work:
val updateListItems = uiState.items.map { state ->
if (state is CheckboxState && state.availableItemType == itemType) {
state.copy(checked = !state.checked)
} else {
state
}
}
val atLeastOneChecked = updateListItems.filterIsInstance<CheckboxState>().find { it.checked } != null
return updateDetailsActionButtonState(updateListItems, enableActionButton = atLeastOneChecked)
Wdyt?
| enableActionButton: Boolean | ||
| ): List<JetpackListItemState> { | ||
| return details.map { state -> | ||
| if (state.type == PRIMARY_ACTION_BUTTON) { |
There was a problem hiding this comment.
❓ Is this equivalent to if (state is ActionButtonState && !state.isSecondary) ? If it is, I think it might be safer to use this approach, wdyt? ( or could we perhaps drop the "isSecondary" check completely?)
There was a problem hiding this comment.
if (state is ActionButtonState) is sufficient ... in this case I can be opinionated because there is only 1 button on the details view. Updated in 14d3a34
There was a problem hiding this comment.
P.S. The changes are not unit tested - I know that the whole class is not unit tested yet, but adding at least tests for the introduced logic might be worth it. Wdyt?
Yes, unit testing is going to be my focus today ... with the builders at the top of the list.
@malinajirka Thanks for the review. :)
Parent #13328 & 13329
This PR addresses the following comment
"On unchecking all checkbox items on the "Details" screen, we might want to disable action button."
By ..
This behavior now mimics Calypso.
To Test
PR submission checklist:
RELEASE-NOTES.txtif necessary.