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

Support required text and optional text configuration. #1963

Merged
merged 16 commits into from
May 18, 2023

Conversation

santosh-pingle
Copy link
Collaborator

@santosh-pingle santosh-pingle commented Apr 12, 2023

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #1954

Description

  • Show required text to the beginning of the instructions if views are non textInputLayout views.
  • Show required text as helper text if view are InputTextLayout views.
  • Support optional text configuration
  • If optional text is configured, then show optional text at the end of the question text.
  • If question text is not present then show optional text as helper text.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: Feature

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@santosh-pingle
Copy link
Collaborator Author

@shelaghm
Required text :
Screenshot 2023-04-13 at 12 30 06 PM

Optional text :
Screenshot 2023-04-13 at 12 24 13 PM

@shelaghm
Copy link

This looks good to me. Thanks @santosh-pingle

@jingtang10
Copy link
Collaborator

  1. I think the behavior between required and option should be the consistent. so if we're appending (optional) after question text when configured to show optional text, we should be appending (required) after quesiton text as well when configured to show required text.
  2. slightly unsure about appending Required. in front of instruction text - Can we just have it as a separate line? happy to be overruled on this one though.

can we just always append (required/optional) after question text, and if question text does't exist, append it to instruction text.

@shelaghm

@shelaghm
Copy link

  1. To be consistent between the two, I recommend we use the instruction text field to append required/optional. In paginated layout the (required) or (optional) text is very large and I think it's not THAT important to display. In default layout it works better than in paginated as the text proportions are more subtle.
  2. Breaking it into two lines is a nice suggestion. @santosh-pingle Do you think we could do a line-break?

I've gone through and mocked up all the options, so we can be sure we are talking about the same options. I'll also post link in group chat as it's a lot!
Screenshot 2023-04-24 at 16 40 13

@shelaghm
Copy link

Feedback from @mberg
To confirm does this mean the sdc will no longer support * or will that be supported as an option? I would want to keep the option to not have to display optional on every question. I feel like it’s a bit overwhelming to see it on every question. Odk and most of the form libraries just use the *. I think a lot of people are used to the pattern but I see why required text is a nice option too. I would want to disable optional on all questions though as I feel that becomes a bit overwhelming.

@santosh-pingle
Copy link
Collaborator Author

@shelaghm
Thanks for sharing the details.
The only pending questions are: what will be the default behavior, and what will be supported from the given table?

@shelaghm
Copy link

shelaghm commented Apr 25, 2023

@santosh-pingle I recommend that the default is 2 - Required and that it is appended to instructions or help text. So that is 2A, 2B, 2C. Default should be separate lines for Required/Optional and additional instructions

I suggest we make the following configurable:

  • Optional (3)
  • Asterisk (1)

I'm not sure if we need to add ability to also append the required/optional text behind the question title instead of in the instructions/help field.

@jingtang10 what do you think? Let's discuss

@shelaghm
Copy link

Team discussion:

  • For edge case: suggest making Required on the second line, instead of it being displayed before the instructions
  • Use Instructions and hint text field
  • Asterisk turns on only the *, in order to also display required text, need to turn on Required option
  • Configurable options:
  • (1) Required text only (default), (2) Optional, (3) Asterisk* - always displayed after question text or inside the text field. They can all be individually toggled on/off.

Note to Shelagh to update the design guidelines with this update.

@santosh-pingle santosh-pingle requested a review from a team as a code owner May 5, 2023 05:15
@santosh-pingle
Copy link
Collaborator Author

@shelaghm

Screenshot 2023-05-05 at 10 56 26 AM

Screenshot 2023-05-05 at 10 56 17 AM

Screenshot 2023-05-05 at 10 56 04 AM

Screenshot 2023-05-05 at 10 55 56 AM

Screenshot 2023-05-05 at 10 55 42 AM

Screenshot 2023-05-05 at 10 55 33 AM

Screenshot 2023-05-05 at 10 55 19 AM

Screenshot 2023-05-05 at 10 55 12 AM

Screenshot 2023-05-05 at 10 54 59 AM

@dubdabasoduba
Copy link
Collaborator

dubdabasoduba commented May 8, 2023

@santosh-pingle I tested all three options and they seem to work well. I however have a small design request for the Asterisk display

Screenshot 2023-05-08 at 14 08 05

I think we need some spacing between the text and the Asterisk. I think the Asterisk should have a red text colour.

cc: @shelaghm

@dubdabasoduba
Copy link
Collaborator

@shelaghm should these 3 options be mutually exclusive? Right now you can how all can be turned on at the same time.

Screenshot 2023-05-08 at 16 39 14

@shelaghm
Copy link

shelaghm commented May 9, 2023

I think we need some spacing between the text and the Asterisk.

Yes, agreed. @santosh-pingle we should add one space before the asterisk.

I think the Asterisk should have a red text colour.

From our usability testing we learned that the red color on the asterisk was confusing, some people thought it was an error because it was red. So I don't recommend making the asterisk red.

should these 3 options be mutually exclusive? Right now you can how all can be turned on at the same time.

Yes, you can turn on all of these at the same time, because you might want to show the required text and the asterisk at the same time.

@dubdabasoduba @santosh-pingle @jingtang10

@mberg
Copy link
Collaborator

mberg commented May 9, 2023 via email

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 11, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1963
  - With unmerged PR google#1907
@santosh-pingle
Copy link
Collaborator Author

Resolving review comments.

@santosh-pingle santosh-pingle enabled auto-merge (squash) May 18, 2023 12:09
@santosh-pingle santosh-pingle merged commit 8f91a5a into google:master May 18, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
6 participants