prefix added to questions #333
Conversation
|
please add a description of your approach here in the PR description. thanks! |
| @@ -56,6 +59,10 @@ internal object QuestionnaireItemCheckBoxViewHolderFactory : | |||
|
|
|||
| override fun bind(questionnaireItemViewItem: QuestionnaireItemViewItem) { | |||
| this.questionnaireItemViewItem = questionnaireItemViewItem | |||
| if (questionnaireItemViewItem.questionnaireItem.prefix.toString().isNotEmpty()) { | |||
There was a problem hiding this comment.
if you're setting visibility, you need to make sure to set it both ways. this is because the same view holder may get recycled and reused for a different data item.
for example, if you have a data item that has prefix, so you set the view holder's prefix to be visiblt, but if the same view holder gets recycled and is bound to a data item that doesn't have prefix, you're not hiding the prefix.
| @@ -27,10 +28,12 @@ internal object QuestionnaireItemCheckBoxViewHolderFactory : | |||
| override fun getQuestionnaireItemViewHolderDelegate() = | |||
| object : QuestionnaireItemViewHolderDelegate { | |||
| private lateinit var checkBox: CheckBox | |||
| private lateinit var prefix: TextView | |||
There was a problem hiding this comment.
throughout this PR, please change the order of the widgets roughly follows the order of display in the actual UI.
in this case put the prefix before checkbox. Also please rename to prefixTextView?
| private lateinit var questionnaireItemViewItem: QuestionnaireItemViewItem | ||
|
|
||
| override fun init(itemView: View) { | ||
| checkBox = itemView.findViewById(R.id.check_box) | ||
| prefix = itemView.findViewById(R.id.prefix) |
There was a problem hiding this comment.
nit: move 1 line above.
| android:id="@+id/prefix" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:visibility="gone" /> |
There was a problem hiding this comment.
do you not need a margin between the prefix and the question iteself?
There was a problem hiding this comment.
yes, otherwise we can't be able to differentiate, and it doesn't look too good either
| android:layout_marginStart="@dimen/item_margin_vertical" | ||
| android:layout_marginEnd="@dimen/item_margin_horizontal" /> |
There was a problem hiding this comment.
shouldn't these be on the linear layout
There was a problem hiding this comment.
they are there as well, but it is there to add some margin between prefix and question
|
With #334 merged, please rebase/merge and run |
# Conflicts: # datacapture/src/androidTest/java/com/google/android/fhir/datacapture/views/QuestionnaireItemEditTextQuantityViewHolderFactoryInstrumentedTest.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemCheckBoxViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemDatePickerViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemDateTimePickerViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemDisplayViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemDropDownViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemEditTextViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemGroupViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemRadioGroupViewHolderFactory.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactory.kt # datacapture/src/main/res/layout/questionnaire_item_check_box_view.xml # datacapture/src/main/res/layout/questionnaire_item_date_picker_view.xml # datacapture/src/main/res/layout/questionnaire_item_date_time_picker_view.xml # datacapture/src/main/res/layout/questionnaire_item_display_view.xml # datacapture/src/main/res/layout/questionnaire_item_drop_down_view.xml # datacapture/src/main/res/layout/questionnaire_item_edit_text_view.xml # datacapture/src/main/res/layout/questionnaire_item_group_header_view.xml # datacapture/src/main/res/layout/questionnaire_item_radio_group_view.xml # datacapture/src/main/res/layout/questionnaire_item_slider.xml
There was a problem hiding this comment.
thanks @MuhammadSalman-7214! can you add a screenshot just so we can see what it looks like now? (i think the my pain questionnaire has some prefixes in it).
you'll need to update the tests (my comments in the first file applies to all test files).
| ) {} | ||
| ) | ||
|
|
||
| assertThat(!viewHolder.itemView.findViewById<TextView>(R.id.prefix).isVisible) |
There was a problem hiding this comment.
this isn't complete. it's not asserting anything. it should be assertThat(...).isFalse().
There was a problem hiding this comment.
okay sure
| } | ||
|
|
||
| @Test | ||
| fun shouldNotSetPrefixText() { |
There was a problem hiding this comment.
| fun shouldNotSetPrefixText() { | |
| fun shouldHidePrefixText() { |
| ) {} | ||
| ) | ||
|
|
||
| assertThat(viewHolder.itemView.findViewById<TextView>(R.id.prefix).text).isEqualTo("Prefix?") |
There was a problem hiding this comment.
also assert on visibility
| @@ -34,6 +36,30 @@ class QuestionnaireItemCheckBoxViewHolderFactoryInstrumentedTest { | |||
| private val parent = FrameLayout(InstrumentationRegistry.getInstrumentation().context) | |||
| private val viewHolder = QuestionnaireItemCheckBoxViewHolderFactory.create(parent) | |||
|
|
|||
| @Test | |||
| fun shouldSetPrefixText() { | |||
There was a problem hiding this comment.
| fun shouldSetPrefixText() { | |
| fun shouldShowPrefixText() { |
# Conflicts: # datacapture/src/main/res/layout/questionnaire_item_radio_group_view.xml
|
|
your welcome @jingtang10 👍 |
No description provided.