Guidance hint solution that resolves #2007 #2105

jd-alexander · 2018-04-08T21:20:12Z

Closes #2007

What has been done to verify that this works as intended?

To verify that this functionality works I manually tested it by creating a form that has guidance attached to the hint of its question. Once that was done, I loaded the form into the Collect app, enabled guidance hints to be shown on questions within the Form Management Settings and opened the form's question which displayed the hint.

To further verify this functionality I wrote an Espresso test that enabled the guidance setting and I created a test instance of FormEntryActivity with the guidance hint form. I then fetched the guidance hint from the respective FormEntryPrompt and compared it with the text of the TextView on the screen that's supposed to perform the rendering.

Why is this the best possible solution? Were any other approaches considered?

This is the best possible solution due to the nature of how the JavaRosa SDK allows text strings with form attributes to be fetched.

Are there any risks to merging this code? If so, what are they?

The only risks associated with merging this code would be the changes that were made to the HelpTextView's layout constraints that were associated with the rendering of the AnswerView. This was being done entirely in code so a xml layout file was used to encapsulate the additional layout that was necessary for the guidance text view. Nonetheless, I made several checks to ensure that these changes didn't break the functionality of the questions and the preexisting UI tests along with the UI test that was written for this feature verifies that this won't cause an impact.

Do we need any specific form for testing your changes? If so, please attach one.

To test this functionality you need a form that has a guidance hint eg.
guidance_hint_form.xml.txt

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

getodk/docs#635

…d constructs. This will allow for the hint and the hint's guidance to live within the same layout. Also added a key to preferences that will be used to store the guidance setting.

…he hint's text id and form type of guidance.

…help text is already gone then it's parent layout won't have a height due to it having the WRAP_CONTENT property so the AnswerView will automatically be positioned beneath the previous layout.

lognaturel

Looking great to me! I've marked a few quick things inline as I saw them but haven't done a full review yet.

Let's wait a day or two for feedback about the UI: https://forum.opendatakit.org/t/collect-collapsible-training-hints-guidance/12247/3

I find the (i) icon very difficult to hit even on a Galaxy Tab A tablet. Is it possible only the actual path is tappable and not the transparent parts? Could we perhaps expand the tappable area?

I wasn't sure initially that I liked the icon but I haven't come up with anything better so it has grown on me.

lognaturel · 2018-04-09T22:03:40Z

collect_app/src/main/res/layout/help_text_layout.xml

+    <TextView
+        android:id="@+id/help_text_view"
+        tools:text="Testing this out."
+        android:gravity="center"


This makes all hint text that wraps centered which doesn't look right. See All Widgets form to see what I mean!

lognaturel · 2018-04-09T22:04:10Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java

@@ -592,4 +677,6 @@ public MediaPlayer getPlayer() {
    public int getPlayColor() {
        return playColor;
    }
+


There shouldn't be any line breaks here.

lognaturel · 2018-04-09T22:04:54Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java

-        TextView helpText = new TextView(getContext());
+    private View createHelpTextLayout() {
+        return LayoutInflater.from(getContext()).inflate(R.layout.help_text_layout, null);
+


Please remove extra line breaks! I saw a few other places where there were extra ones so would be good to double check the diff.

No prob :) will definitely keep an eye out for them!

lognaturel · 2018-04-09T22:05:16Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java

+    }
+
+    private TextView setupGuidanceTextAndLayout(TextView guidanceTextView, FormEntryPrompt prompt) {
+


Our style doesn't call for a line break here.

lognaturel · 2018-04-09T23:53:50Z

collect_app/src/main/res/values/strings.xml

@@ -584,4 +587,5 @@
    <string name="start_video_capture_instruction">Tap the screen to start recording</string>
    <string name="stop_video_capture_instruction">Recording started. Tap again to stop</string>
    <string name="no_columns_to_upload">No columns found in the form to upload.</string>
+    <string name="guidance_hint_title">Show Guidance For Questions</string>


Should be in sentence case ("Show guidance for questions").

lognaturel · 2018-04-09T23:54:21Z

collect_app/src/main/res/values/strings.xml

@@ -495,6 +495,9 @@
    <string name="user_access_control">User Access Control</string>
    <string name="autosend_selector_title">Auto send</string>
    <string name="off">Off</string>
+    <string name="guidance_no">No</string>
+    <string name="guidance_yes">Yes - Always Shown</string>


always shown (all lowercase)

lognaturel · 2018-04-09T23:54:39Z

collect_app/src/main/res/values/strings.xml

@@ -495,6 +495,9 @@
    <string name="user_access_control">User Access Control</string>
    <string name="autosend_selector_title">Auto send</string>
    <string name="off">Off</string>
+    <string name="guidance_no">No</string>
+    <string name="guidance_yes">Yes - Always Shown</string>
+    <string name="guidance_yes_collapsed">Yes - Collapsed</string>


collapsed (all lowercase)

jd-alexander · 2018-04-10T00:51:53Z

Thanks for the review!
Icon/UI - Yes I am not too sure about it either so I just put it in place until we could all decide on something together. I have an idea to spruce up the UI a bit so I am gonna make those changes and share them here.

Tappable area

I find the (i) icon very difficult to hit even on a Galaxy Tab A tablet. Is it possible only the actual path is tappable and not the transparent parts? Could we perhaps expand the tappable area?

Yes, we could make clicks only take place on the actual path. Currently I just have an OnClickListener on the TextView because I am using it's CompoundDrawable mechanism so no matter the length of the hint the icon will always be properly aligned at the end of the text. Started working on a layout with a TextView and ImageView but I just thought this approach would yield the same results.

lognaturel · 2018-04-10T01:51:34Z

Currently I just have an OnClickListener on the TextView

Of course, I hadn't noticed that! That explains why I was having a tough time when I tried to tap the (i). Eventually it would be good to have both text and icon be tappable but let's wait for some preliminary UI feedback. 😊

jd-alexander · 2018-04-12T07:40:22Z

Okay :) I made just the drawable clickable for now. Gonna do some thorough testing of it to see how it behaves.

yanokwa · 2018-04-13T16:43:57Z

gradle/wrapper/gradle-wrapper.properties

I think this change should be reverted because we don't need the sources and documentation that all comes with. Why download things we don't need 😂

haha no prob. Never knew that's what the all meant. Thanks for pointing that out.

yanokwa · 2018-04-13T16:54:58Z

I think this works really nicely, and we have tests! Hard to do complain about that...

Some changes I'd like to see before this goes to testing.

The i icon should probably be black. Gray makes it look disabled.
The icon isn't a huge touch surface, so maybe we can make the whole hint text clickable, to make life easier for users? This is similar to how you can select the entire line for a radio button in selects ones.

I'm not a huge fan of the i after the text (although @lognaturel prefers it!). I had similar pushback when we added the red star for required questions (#1828 (review)) and I was able to convince folks to move the red star to the beginning. I'm not necessarily arguing for a change, but I'm curious how you decided to put it at the end, @jd-alexander.

jd-alexander · 2018-04-16T15:11:09Z

Good looking out @yanokwa . Honestly, I was used to putting asterisks/icons about hints at the end of the text from my old job so that's the reason I placed it there. I did some UX research on this topic and I found some detailed explanations from the folks over at UX Stack Exchange that justifies it being placed before the text so I am gonna opt for that approach because it makes more sense and it actually lessens most of the issues I had to deal with such as the icon trailing the text and causing inconsistencies with other hint guidance's that might be shorter. I am gonna work on this change and a subtle effect to make this more appealing and share it here 😄

yanokwa · 2018-04-24T19:21:11Z

@jd-alexander If you can get those changes sent in soon and fix the conflicts, I think we'd be ready to test and merge!

jd-alexander · 2018-04-25T05:07:19Z

@yanokwa Yes I am gonna finish this up! Was just a bit pre occupied with the SMS stuff but yes I want to finalize this so I will spend some time doing this tomorrow!

jd-alexander · 2018-04-25T19:46:31Z

@yanokwa I was thinking of adding a background to the guidance hint when it's in collapse mode so when it gets expanded it has a bit more body and outline. I have this working example in my repo of how it would look when it's open. What do you think?
I included this in the pushes I sent so you can take a look if necessary.

…line is displayed when it's expanded in collapsed mode.

lognaturel

Few small things and this will be ready for QA!

The guidance hint shouldn't disappear on rotate
The hint font size is already small so I would like guidance hints to have the same font size

I don't find the box around the guidance hint necessary but I don't feel very strongly about it. @yanokwa?

lognaturel · 2018-04-30T23:03:27Z

collect_app/src/main/java/org/odk/collect/android/preferences/PreferenceKeys.java

@@ -21,6 +21,7 @@
    public static final String KEY_CONSTRAINT_BEHAVIOR      = "constraint_behavior";
    public static final String KEY_HIGH_RESOLUTION          = "high_resolution";
    public static final String KEY_IMAGE_SIZE               = "image_size";
+    public static final String KEY_GUIDANCE_HINT               = "guidance_hint";


Indentation here should match that of other values.

lognaturel · 2018-04-30T23:18:43Z

collect_app/src/main/java/org/odk/collect/android/preferences/GuidanceHint.java

+    public static GuidanceHint get(String name) {
+        for (GuidanceHint hint : GuidanceHint.values()) {
+            if (hint.name.equals(name)) {
+


Remove extra blank line.

lognaturel · 2018-04-30T23:18:55Z

collect_app/src/main/java/org/odk/collect/android/utilities/AnimateUtils.java


 /**
 * Created by Ing. Oscar G. Medina Cruz on 18/06/2016.
 */
 public class AnimateUtils {

+


Remove extra blank line

lognaturel · 2018-04-30T23:19:03Z

collect_app/src/main/java/org/odk/collect/android/utilities/AnimateUtils.java

@@ -39,4 +47,114 @@ public void onAnimationRepeat(Animation animation) {
        });
        view.startAnimation(scaleInAnimation);
    }
+
+


Remove extra blank line

lognaturel · 2018-04-30T23:20:00Z

collect_app/src/main/java/org/odk/collect/android/utilities/AnimateUtils.java

+        animation.setAnimationListener(new Animation.AnimationListener() {
+            @Override
+            public void onAnimationStart(Animation animation) {
+


Remove extra blank line

lognaturel · 2018-04-30T23:20:32Z

collect_app/src/main/java/org/odk/collect/android/utilities/UiUtils.java

+
+
+    /**
+     * This method converts dp unit to equivalent pixels, depending on device density.


Remove "This method"

lognaturel · 2018-04-30T23:20:43Z

collect_app/src/main/java/org/odk/collect/android/utilities/UiUtils.java

+    }
+
+    /**
+     * This method converts device specific pixels to density independent pixels.


Remove "This method"

lognaturel · 2018-04-30T23:21:02Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java

+
+        if (android.text.TextUtils.isEmpty(guidanceHint)) {
+            return null;
+


Remove extra blank line

lognaturel · 2018-04-30T23:21:17Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java

-    /** Releases resources held by this widget */
+
+    private TextView configureGuidanceTextView(TextView guidanceTextView, String guidance) {
+


Remove extra blank line

lognaturel · 2018-04-30T23:21:23Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java

+        guidanceTextView.setTextColor(ContextCompat.getColor(getContext(), R.color.primaryTextColor));
+        guidanceTextView.setMovementMethod(LinkMovementMethod.getInstance());
+        return guidanceTextView;
+


Remove extra blank line

yanokwa · 2018-05-01T00:09:26Z

I don't love the box.

jd-alexander · 2018-05-01T00:12:51Z

Okay no prob! Will remove the box!

lognaturel · 2018-05-01T00:24:08Z

Awesome! Once you've done the changes and the merge conflicts are resolved (sorry!), you can add the needs-testing tag with @opendatakit-bot label needs-testing and it will go through the QA process.

… Performed several style fixes.

….Other minor fixes

jd-alexander · 2018-05-01T03:38:30Z

@opendatakit-bot label needs-testing

jd-alexander · 2018-05-01T03:43:13Z

Removed box outline
Added collapsed state to saved state
Integrated control with dark theme
Corrected style errors

Ready for testing 🎆

mmarciniak90 · 2018-05-08T14:04:46Z

@jd-alexander I noticed a problem in case:

User installs the application (Show guidance for questions is set as a No by default)
User starts Guidance Form Sample form
User goes to General Settings
User changes Show guidance for questions to Yes - collapsed
User backs to form
User sees error

Additionally, i icon is cut off on devices with >5 inches screen

lognaturel · 2018-05-08T16:48:42Z

Good catch, @mmarciniak90! @jd-alexander, hopefully these are relatively quick fixes. Would be great to get this in a beta by Thursday!

jd-alexander · 2018-05-08T17:44:15Z

Thanks for catching these @mmarciniak90 @lognaturel They should be! Working on them now.

The hint guidance was being attached by using CompoundDrwables of TextView. The issue with using this was that there wasn't alot of flexibility with the layout attributes that could be applied to the drawable eg. padding can be applied (which was causing the bug) but not margins so I removed the drawable and created a layout to host the guidance textview and an an imageview for the icon

jd-alexander · 2018-05-08T20:12:55Z

@mmarciniak90 resolved the issues! Ready for another test to verify.

mmarciniak90 · 2018-05-09T10:08:49Z

@jd-alexander mentioned problems are resolved!
I checked more question types and I think that repeating guidance for Full date and time question type is not expected.

You can use form: hints.xml.txt

The TimeWidget and DateWidget's help layout was hidden so that only the parent widget's layout is displayed. Previous only the help text was being hidden thus these changes had to take place due to the introduction of the new layout structure for hint guidance.

jd-alexander · 2018-05-09T11:33:28Z

@mmarciniak90 Resolved and ready to go!

mmarciniak90 · 2018-05-10T09:14:22Z

Tested with success!

@jd-alexander great work! Everything looks great!

Verified on Android 4.1, 4.2, 4.4., 5.1 6.0, 7.0, 8.1

Verified cases:

clear settings
many types of question
full date and time question
many questions on one page
dark theme

@opendatakit-bot label "behavior verified"
@opendatakit-bot unlabel "needs testing"

jd-alexander added 9 commits April 7, 2018 14:29

created a layout and view binding mechanisms for all help/hint relate…

b5fb693

…d constructs. This will allow for the hint and the hint's guidance to live within the same layout. Also added a key to preferences that will be used to store the guidance setting.

added list preference for guidance setting

a729e9a

fetched guidance hint from the associated FormEntryCaption based on t…

0257eeb

…he hint's text id and form type of guidance.

added animations to collapse guidance setup.

c29f833

wrote integration tests to verify that guidance hint works.

f6b42dc

performed cleanup of test.

6fc91cf

added styling and configurations for rendering to guidance text.

fa67b0f

re-enabled D8.

9449e31

removed this visibility check on help related layouts because if the …

66b5cd2

…help text is already gone then it's parent layout won't have a height due to it having the WRAP_CONTENT property so the AnswerView will automatically be positioned beneath the previous layout.

jd-alexander mentioned this pull request Apr 8, 2018

Guidance Hint example for ODK Collect getodk/docs#635

Open

jd-alexander added 5 commits April 8, 2018 16:40

resolved pmd issues.

eadb0ad

fixed checkstyle issues

c7b8f7d

performed several fixes to adhere to codacy styles

39df324

changed assertTrue to assertFalse for string null check.

2254319

added view id matcher static import.

368ea98

lognaturel reviewed Apr 10, 2018

View reviewed changes

jd-alexander force-pushed the guidance_hint branch 2 times, most recently from b5e591c to 5ccab73 Compare April 12, 2018 18:37

performed style and drawable click fixes

5ccab73

yanokwa reviewed Apr 13, 2018

View reviewed changes

jd-alexander added 2 commits April 25, 2018 15:21

added a background to guidance hint's layout so that a noticeable out…

8c77dc9

…line is displayed when it's expanded in collapsed mode.

Merge branch 'master' into guidance_hint

c7062fc

minor fix to trigger rebuild.

3f7a0d4

lognaturel reviewed Apr 30, 2018

View reviewed changes

added the ability to store expanded/collapsed state of guidance hint.…

80e417c

… Performed several style fixes.

jd-alexander force-pushed the guidance_hint branch 2 times, most recently from 08bfec2 to c1f5ab7 Compare May 1, 2018 02:39

jd-alexander added 3 commits April 30, 2018 21:46

renamed collapsed key to expanded to match property it's refernencing…

c1f5ab7

….Other minor fixes

integrated guidance hint with dark/light theme functionality

32d00a6

Merge branch 'master' into guidance_hint

d3b8278

lognaturel added the needs testing label May 1, 2018

jd-alexander force-pushed the guidance_hint branch from bebfd39 to 48164cf Compare May 8, 2018 20:04

Merge branch 'master' into guidance_hint

10ca320

jd-alexander added 2 commits May 9, 2018 06:23

Merge remote-tracking branch 'origin/guidance_hint' into guidance_hint

64b99b0

getodk-bot added behavior verified and removed needs testing labels May 10, 2018

lognaturel approved these changes May 10, 2018

View reviewed changes

lognaturel merged commit 5deb17a into getodk:master May 10, 2018

		}

		private TextView setupGuidanceTextAndLayout(TextView guidanceTextView, FormEntryPrompt prompt) {



		/**
		* This method converts dp unit to equivalent pixels, depending on device density.


		if (android.text.TextUtils.isEmpty(guidanceHint)) {
		return null;

		/** Releases resources held by this widget */

		private TextView configureGuidanceTextView(TextView guidanceTextView, String guidance) {

Guidance hint solution that resolves #2007 #2105

Guidance hint solution that resolves #2007 #2105

Conversation

jd-alexander commented Apr 8, 2018 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

Are there any risks to merging this code? If so, what are they?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jd-alexander commented Apr 10, 2018 • edited Loading

lognaturel commented Apr 10, 2018

jd-alexander commented Apr 12, 2018

yanokwa Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanokwa commented Apr 13, 2018

jd-alexander commented Apr 16, 2018 • edited Loading

yanokwa commented Apr 24, 2018 • edited Loading

jd-alexander commented Apr 25, 2018

jd-alexander commented Apr 25, 2018 • edited Loading

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanokwa commented May 1, 2018

jd-alexander commented May 1, 2018

lognaturel commented May 1, 2018

jd-alexander commented May 1, 2018

jd-alexander commented May 1, 2018

mmarciniak90 commented May 8, 2018 • edited Loading

lognaturel commented May 8, 2018

jd-alexander commented May 8, 2018

jd-alexander commented May 8, 2018

mmarciniak90 commented May 9, 2018

jd-alexander commented May 9, 2018

mmarciniak90 commented May 10, 2018

jd-alexander commented Apr 8, 2018 •

edited

Loading

jd-alexander commented Apr 10, 2018 •

edited

Loading

yanokwa Apr 13, 2018 •

edited

Loading

jd-alexander commented Apr 16, 2018 •

edited

Loading

yanokwa commented Apr 24, 2018 •

edited

Loading

jd-alexander commented Apr 25, 2018 •

edited

Loading

mmarciniak90 commented May 8, 2018 •

edited

Loading