chore: tags ui refactor (WPB-22984) #4467
Conversation
sbakhtiarov
force-pushed
the
chore/tags-ui-refactor
branch
2 times, most recently
from
1201ec6 to
cbcedab
Compare
sbakhtiarov
force-pushed
the
chore/tags-ui-refactor
branch
from
cbcedab to
83b8ff6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4467 +/- ##
===========================================
+ Coverage 48.46% 48.48% +0.01%
===========================================
Files 575 575
Lines 19803 19804 +1
Branches 3313 3311 -2
===========================================
+ Hits 9598 9601 +3
+ Misses 9190 9189 -1
+ Partials 1015 1014 -1
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
sbakhtiarov
changed the title
|
| addedTags.forEach { item -> | ||
| WireFilterChip( | ||
| modifier = Modifier.align(Alignment.CenterVertically), | ||
| label = item, | ||
| isSelected = true, | ||
| onSelectChip = onRemoveTag | ||
| ) | ||
| } |
There was a problem hiding this comment.
Can we wrap each chip with key(tag) to avoid state and animation issues when the list changes?
addedTags.forEach { item ->
key(item) {
WireFilterChip(
modifier = Modifier.align(Alignment.CenterVertically),
label = item,
isSelected = true,
onSelectChip = onRemoveTag
)
}
}
| fun updateViewState() { | ||
| updateViewState(state.value.addedTags) | ||
| } | ||
|
|
||
| result | ||
| .onSuccess { sendAction(AddRemoveTagsViewModelAction.Success) } | ||
| .onFailure { sendAction(AddRemoveTagsViewModelAction.Failure) } | ||
| .also { isLoading.value = false } | ||
| fun updateViewState(addedTags: Set<String>) { |
There was a problem hiding this comment.
We could remove redundant function overload by making it like this
fun updateViewState(addedTags: Set<String> = state.value.addedTags)
And can we cover it with unit test
|
|
||
| private val navArgs: AddRemoveTagsNavArgs = savedStateHandle.navArgs() | ||
| private val initialTags: Set<String> = navArgs.tags.toSet() | ||
| private val disallowedChars = listOf(",", ";", "/", "\\", "\"", "\'", "<", ">") |
There was a problem hiding this comment.
| private val disallowedChars = listOf(",", ";", "/", "\\", "\"", "\'", "<", ">") | |
| private val disallowedChars = setOf(",", ";", "/", "\\", "\"", "\'", "<", ">") |
| tag.trim().let { newTag -> | ||
| if (newTag.isNotBlank() && newTag !in addedTags.value) { | ||
| addedTags.update { it + newTag } | ||
| updateSuggestions("") | ||
| val addedTags = state.value.addedTags | ||
| if (newTag.isNotBlank() && newTag !in addedTags) { | ||
| updateViewState(addedTags + tag) | ||
| tagsTextState.clearText() | ||
| } | ||
| } |
There was a problem hiding this comment.
we could avoid unnecessary let here
val newTag = tag.trim()
if (newTag.isNotBlank() && newTag !in state.value.addedTags) {
updateViewState(state.value.addedTags + newTag)
tagsTextState.clearText()
}
| launch { | ||
| snapshotFlow { tagsTextState.text.toString() } | ||
| .debounce(TYPING_DEBOUNCE_TIME) | ||
| .collectLatest { updateViewState() } | ||
| } |
There was a problem hiding this comment.
The inner launch is unnecessary.
The flow collection can run directly in the outer coroutine, since we always need to wait for getAllTagsUseCase() to complete before collecting the flow.
https://wearezeta.atlassian.net/browse/WPB-22984
What's new in this PR?
Fixed issues:
Other: