[Multiple Choice Task] Fix UI bugs in multiple choice selectors #2899
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2899 +/- ##
============================================
- Coverage 62.22% 61.90% -0.33%
+ Complexity 1205 1204 -1
============================================
Files 267 268 +1
Lines 6380 6441 +61
Branches 882 897 +15
============================================
+ Hits 3970 3987 +17
- Misses 1863 1898 +35
- Partials 547 556 +9
|
|
Marking this as draft. Working towards migrating the task's UI to compose. |
shobhitagarwal1612
force-pushed
the
ashobhit/2893/choice-ux
branch
2 times, most recently
from
2adc6a8 to
459bee0
Compare
|
@gino-m This is now ready for another pass! Migrated the UI to compose and updated styling & unit tests. |
| isLastIndex: Boolean = false, | ||
| toggleItem: (item: MultipleChoiceItem) -> Unit = {}, | ||
| otherValueChanged: (text: String) -> Unit = {}, |
There was a problem hiding this comment.
how about have state and events
something like
val _multiplaeChoiceState = MutableStateFlow<MultiplaeChoiceData>(MultiplaeChoiceData())val _multiplaeChoiceEvent = MutableSharedFlow<MultiplaeChoiceEvents>()There was a problem hiding this comment.
Are there any benefits to using sharedFlow in this case?
There was a problem hiding this comment.
- all events can be managed at a single place
- can also prevent the double click
- no multiple parameters to compose functions
There was a problem hiding this comment.
I tried that but the tests started failing after that. Can we handle that separately?
| RadioButton( | ||
| modifier = Modifier.testTag(MultipleChoiceTestTags.SELECT_MULTIPLE_RADIO), | ||
| selected = item.isSelected, | ||
| onClick = { toggleItem(item) }, |
There was a problem hiding this comment.
and then we can emit like
multiplaeChoiceEvent(MultiplaeChoiceEvent.ToggleClick(item))| */ | ||
| package com.google.android.ground.ui.datacollection.tasks.multiplechoice | ||
|
|
||
| object MultipleChoiceTestTags { |
There was a problem hiding this comment.
we don't need object, right?
we can keep constants as is
There was a problem hiding this comment.
IMO, wrapping this under object makes it easier to use the constants
There was a problem hiding this comment.
easier to use but object MultipleChoiceTestTags will create a extra memory, object creation
in terms of using, we can directly write MULTIPLE_CHOICE_LIST no object name reference required
There was a problem hiding this comment.
MultipleChoiceTestTags is a singleton, so the memory for the object reference is minimal, if not zero if optimized out by the compiler.
There was a problem hiding this comment.
According to Gemini, top-level declarations are preferred in Kotlin, but haven't validated this yet:
- Top-level declarations are preferred: Kotlin encourages the use of top-level functions and properties, which naturally provide a way to organize code into logical units without creating unnecessary objects.
- Readability and maintainability: Using objects as namespaces can make your code harder to read and understand. Top-level declarations are more straightforward and easier to locate.
- File facade issues (JVM): On the JVM, Kotlin compiles top-level declarations into a file facade class. If you use objects as namespaces, you might encounter naming conflicts with these generated classes.
- Top-level functions and properties: The preferred way to group related code is to use top-level functions and properties within the same file.
- Packages: For larger projects, use packages to organize your code into meaningful modules.
Fixes #2893
Updates UX as per figma mocks
@gino-m PTAL?