Skip to content
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

Add hint text to QueryRequestUIController #2668

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Charl1996
Copy link
Contributor

@Charl1996 Charl1996 commented May 1, 2023

Summary

Ticket

This PR is to align the webapp and Android app Case Claim UX by adding a hint button on Android which brings up a dialog when clicked, showing the hint/help text configured on HQ, similar to what is seen on the webapps.

When no help/hint text is configured, the hint icon is not shown.

Feature Flag

None specific.

Product Description

The image below shows what the user would have seen before this PR (as well as in the case that no hint text is configured),

without_hint_icon

When hint text is configured on HQ, the user will see the following button,

hint_icon

Now when the user clicks on the button, a dialog appears with the configured hint text, as seen below,

hint_popup

Safety Assurance

  • If the PR is high risk, "High Risk" label is set
  • I have confidence that this PR will not introduce a regression for the reasons below
  • Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Automated test coverage

Change not covered in automated tests (don't think it's necessary?).

Safety story

Did local testing with and without hint text configured.

@Charl1996 Charl1996 requested a review from shubham1g5 May 1, 2023 20:15
promptHelpButton.visibility = View.VISIBLE

promptHelpButton.setOnClickListener {
AlertDialogWrapper.alertDialog(queryRequestActivity, "Hint", hintText)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curios what you think about removing the wrapper and directly calling the StandardAlertDialog.getBasicAlertDialog() method from here.

@@ -18,6 +18,15 @@
android:gravity="center|left"
android:layout_height="wrap_content" />

<ImageButton
android:id="@+id/prompt_hint_button"
android:layout_width="0dp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious what happens if we set the width here to wrap_content and remove layout_weight from below ? I want to avoid using layout_weight for help icon since it may result in unexpected behaviours for different screen sizes. We are setting layout_weight on other views to 1 since we want them to take all the available space but this is not true for help view.

@Charl1996 Charl1996 requested a review from shubham1g5 May 10, 2023 13:01
@@ -20,7 +20,7 @@

<ImageButton
android:id="@+id/prompt_hint_button"
android:layout_width="0dp"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_weight="0.5"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to remove layout_weight if we are able to set the layout_width to wrap_content

@@ -20,7 +20,7 @@

<ImageButton
android:id="@+id/prompt_hint_button"
android:layout_width="0dp"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_weight="0.5"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to remove layout_weight if we are able to set the layout_width to wrap_content

@shubham1g5
Copy link
Contributor

Thanks @Charl1996 . This looks good to me. Although I think we need to do 2 other things to finalise this PR-

  1. Merge master in the current branch to get the build passing on this PR
  2. Add a test possibly as part of this roboelectric test to test that hint is visible for one of the views when it's configured in the app. You will need to manually modify the test ccz to add the hint xml for one of the case search input field.

@Charl1996
Copy link
Contributor Author

Thanks @Charl1996 . This looks good to me. Although I think we need to do 2 other things to finalise this PR-

  1. Merge master in the current branch to get the build passing on this PR
  2. Add a test possibly as part of this roboelectric test to test that hint is visible for one of the views when it's configured in the app. You will need to manually modify the test ccz to add the hint xml for one of the case search input field.

Thanks! Will do

@@ -217,6 +218,12 @@ public void reloadQueryActivityStateTest() {
EditText patientName = promptsLayout.getChildAt(0).findViewById(R.id.prompt_et);
assertEquals("francisco", patientName.getText().toString());

ImageButton imageButtonWithoutHint = promptsLayout.getChildAt(0).findViewById(R.id.prompt_hint_button);
assertEquals(8, imageButtonWithoutHint.getVisibility());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the constants from view class here like View.VISIBLE instead of using raw int values, but looks good otherwise.

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just have one final comment that I didn't notice initially.

val displayData = queryPrompt.display.evaluate()
val hintText = displayData.hintText ?: return "";

return Localizer.processArguments(hintText, arrayOf("")).trim { it <= ' ' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to call Localizer. processArguments here as calling queryPrompt.display.evaluate() should evaluate any localised strings itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants