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

Display errors inline rather than in toast #5725

Merged
merged 47 commits into from
Dec 7, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Sep 5, 2023

Closes #5618
Closes #5751

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

I've tested the changes manually and fixed automated tests.

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

This is what we agreed on in the issue and when it comes to the implementation there is nothing to discuss here.

The final solution differs a little bit from what was described in the issue so let me list the changes that we introduced:

  • Added a red outline with rounded corners to highlight the error. The outline should be displayed no matter what the question type is but there should be a small difference in displaying the error message when it's a text question. See the issue.
  • Removed the red animation that we used to highlight required/invalid answers when there were multiple questions on the same page
  • Updated the look of text fields. See: Display errors inline rather than in toast #5725 (comment) that's why testing read-only text widgets (plus external widgets and the Bearing widget too) is so important. See Fix spacing for read-only text widget  #5751

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Please test how different widgets look like when they are required but there is no answer or when constraints are violated. It's also important to keep in mind that apart from the default error messages we have when an error is displayed we can set custom ones too using the required_message and constraint_message columns in xls (see https://xlsform.org/en/#required-message and https://xlsform.org/en/#constraint-message)

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

I used the All widgets form but with all questions required to test every single widget:
All widgets.xml.txt

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010
Copy link
Member Author

@alyblenkin, it would be good to have early feedback from you. You can download the apk from CircleCI and here I'm attaching two forms I've used so far:
all-widgets.xlsx - all questions are required so you can see how it looks like with different widgets.
RequiredAndConstraint.xlsx - it contains only 4 questions and two of them have custom messages (see https://xlsform.org/en/#required-message and https://xlsform.org/en/#constraint-message)

I've already noticed that some widgets will need to be updated because even if they do not have answers they still keep room for them visible for example:

Screenshot_1693920480

List widgets also would need to be updated because they don't have any margin end/right and some elements are visible out of the red outline:

Screenshot_1693920509

what do you think about those two? Any other thoughts?

@alyblenkin
Copy link
Collaborator

Hey @grzesiek2010, I haven't been feeling well so I haven't gone through all of the widgets, but it's going in the right direction! My first reaction was we need a bit more padding between the question and the red line because it feels a bit squishy. However, we don't want the error to take up too much space, especially for smaller phones – so what you've done works.

I agree we would want to reduce the spacing on the ones that don't have answers. And fix the margin for the list widgets, so things don't overlap. For text field errors, I thought we should use https://m3.material.io/components/text-fields/specs#f157ea73-f2fd-4d99-aab3-c96bd9c91eec

A few interaction things I noticed:

The error doesn't go away once you've addressed the error in some cases, like when I uploaded the image.

Screenshot_20230905-231445_ODK Collect

I also noticed the error message disapears when you rotate the device. But it works if you have the device already rotated and try to go ahead without answering the question.

I assume this is another issue, but the toast message is truncated here, which isn't ideal.

Screenshot_20230905-233208_ODK Collect

@grzesiek2010
Copy link
Member Author

I assume this is another issue, but the toast message is truncated here, which isn't ideal.

I'm not sure if this is related to this issue that was supposed to be about required questions and constraints. That toast is displayed when a user tries to start an external app (using external widgets) or widgets like image/video but for some reason, there is no app installed on their device that can handle it. Probably it's something you can explore later and maybe we should get rid of those toasts too and display them in a similar way like constraints but it's a separate thing I would say.

@grzesiek2010
Copy link
Member Author

For text field errors, I thought we should use https://m3.material.io/components/text-fields/specs#f157ea73-f2fd-4d99-aab3-c96bd9c91eec

Yeah I know but in the issue, we discussed implementing the same style for all widgets first and then (maybe in a separate pr) handling text widgets.

@grzesiek2010
Copy link
Member Author

My first reaction was we need a bit more padding between the question and the red line because it feels a bit squishy

I thought we wanted to avoid increasing the margins, and I haven't done that; I've simply introduced an outline between the screen edge and the questions. Do you think the outline should be shifted slightly towards the screen edge without expanding the margin between the edge and the questions? Or maybe you want to add more margin?

@grzesiek2010 grzesiek2010 force-pushed the COLLECT-5618 branch 2 times, most recently from 78813ea to 29e5ded Compare September 6, 2023 11:26
@alyblenkin
Copy link
Collaborator

My first reaction was we need a bit more padding between the question and the red line because it feels a bit squishy

I thought we wanted to avoid increasing the margins, and I haven't done that; I've simply introduced an outline between the screen edge and the questions. Do you think the outline should be shifted slightly towards the screen edge without expanding the margin between the edge and the questions? Or maybe you want to add more margin?

Yes, we absolutely want to avoid increasing the margins. If it isn't too much work, I would be curious to see what it looks like slightly shifted towards the screen edge, but I also think what you have works!

@alyblenkin
Copy link
Collaborator

I assume this is another issue, but the toast message is truncated here, which isn't ideal.

I'm not sure if this is related to this issue that was supposed to be about required questions and constraints. That toast is displayed when a user tries to start an external app (using external widgets) or widgets like image/video but for some reason, there is no app installed on their device that can handle it. Probably it's something you can explore later and maybe we should get rid of those toasts too and display them in a similar way like constraints but it's a separate thing I would say.

Yes, I think it's a different issue as well. I agree we should do away with the toast and make it more actionable as well, so the user knows what to do to resolve it. I'll add this to my backlog to investigate!

@grzesiek2010 grzesiek2010 force-pushed the COLLECT-5618 branch 2 times, most recently from 8debc71 to 2d7058b Compare September 7, 2023 09:41
@grzesiek2010
Copy link
Member Author

The error doesn't go away once you've addressed the error in some cases, like when I uploaded the image.

Handling this properly (deciding if the error should be still visible or not) would require form validation every time we receive a new answer. I'm not sure if it's a good idea because it would slow down the process. Maybe just removing the error in such cases would be enough (when we start an external app) then when a user is back the error would appear again on moving forward (if needed)?
But I'm not sure... I'm also ok with leaving this as-is. If you have other types of questions the error message won't disappear immediately when for example in select_one question you select something so why would we need to do that for widgets that start external apps?

@grzesiek2010
Copy link
Member Author

For text field errors, I thought we should use https://m3.material.io/components/text-fields/specs#f157ea73-f2fd-4d99-aab3-c96bd9c91eec

You can test it now @alyblenkin. There is only one thing... I used outlined text fields because I think they look better. You can install the app and let me know if you agree.

@grzesiek2010
Copy link
Member Author

I agree we would want to reduce the spacing on the ones that don't have answers. And fix the margin for the list widgets, so things don't overlap.

This is also ready so you can test it if you want.

@alyblenkin
Copy link
Collaborator

The error doesn't go away once you've addressed the error in some cases, like when I uploaded the image.

Handling this properly (deciding if the error should be still visible or not) would require form validation every time we receive a new answer. I'm not sure if it's a good idea because it would slow down the process. Maybe just removing the error in such cases would be enough (when we start an external app) then when a user is back the error would appear again on moving forward (if needed)? But I'm not sure... I'm also ok with leaving this as-is. If you have other types of questions the error message won't disappear immediately when for example in select_one question you select something so why would we need to do that for widgets that start external apps?

I can see how that would slow down performance. I think removing the error would be helpful feedback for any type of question, so the user knows it's no longer invalid. I think this wasn't as obvious before because we just had the toast, now it's very clear something is wrong and the expected behaviour would be that it goes away once I've addressed the error.

@alyblenkin
Copy link
Collaborator

alyblenkin commented Sep 7, 2023

You can test it now @alyblenkin. There is only one thing... I used outlined text fields because I think they look better. You can install the app and let me know if you agree.

Testing this now :) I think the outline is growing on me and it's super clear what the user needs to engage with! We may want to add a bit more spacing between the question and the text field because it's a bit tight.

Screenshot_20230907-163529_ODK Collect

I also noticed the outline text field is nearly touching the red error border here
Screenshot_20230907-163124_ODK Collect

I also don't think the list widget was updated because everything is still overlapping.

@alyblenkin
Copy link
Collaborator

I agree we would want to reduce the spacing on the ones that don't have answers. And fix the margin for the list widgets, so things don't overlap.

This is also ready so you can test it if you want.

This looks much better!

@alyblenkin
Copy link
Collaborator

@grzesiek2010 I was wondering if we could also try using the surface container for the text field? For the text field colour, we could use light grey. I think that might be a bit more subtle than the outline.

This will also help us move towards our goal of creating consistency across the tools. Central uses this pattern already, and we would look to the same with Enketo. Central doesn't have a small radius on the corners, but we could move in that direction so everything is rounded.

Screenshot 2023-09-07 at 2 08 57 PM

@grzesiek2010
Copy link
Member Author

@grzesiek2010 I was wondering if we could also try using the surface container for the text field? For the text field colour, we could use light grey. I think that might be a bit more subtle than the outline.

Done. You can test it.

@grzesiek2010
Copy link
Member Author

I also don't think the list widget was updated because everything is still overlapping.

I only added the margin end so that the red outline doesn't overlap the labels. Those list widgets have labels and choices placed horizontally and are rather supposed to be used on larger screens otherwise the choices/labels will overlap.

@grzesiek2010
Copy link
Member Author

I can see how that would slow down performance. I think removing the error would be helpful feedback for any type of question, so the user knows it's no longer invalid. I think this wasn't as obvious before because we just had the toast, now it's very clear something is wrong and the expected behaviour would be that it goes away once I've addressed the error.

I've added removing the error every time the answer changes (no matter what widget it is) please play with it and let me know if you like it. The fact that there is a new answer doesn't mean there should be no error. We would need to validate constraints to check it but maybe it's better than assuming that the new answer is still wrong and removing the error only after navigating forward. So basically there is no perfect way but I think I like what I've implemented now a little bit more.

@grzesiek2010
Copy link
Member Author

I also noticed the error message disapears when you rotate the device

If you asked me to fix this a few years ago I would say of course but my thinking has changed a little bit over the past few years and now I'm more into keeping only crucial things like answers etc. after rotating a device. We have talked about that with @seadowg a few times and I think we were generally on the same page.
@seadowg is this error something that you would rather keep or you agree it's not a must and maybe we can add it later if we change our mind?

@alyblenkin
Copy link
Collaborator

@grzesiek2010 I was wondering if we could also try using the surface container for the text field? For the text field colour, we could use light grey. I think that might be a bit more subtle than the outline.

Done. You can test it.

Thanks so much for trying this out! I prefer this option over the outline. Could we make the grey a bit lighter? Perhaps the same colour as the main menu buttons?

The spacing between the question and text field looks a lot better too!

@alyblenkin
Copy link
Collaborator

I can see how that would slow down performance. I think removing the error would be helpful feedback for any type of question, so the user knows it's no longer invalid. I think this wasn't as obvious before because we just had the toast, now it's very clear something is wrong and the expected behaviour would be that it goes away once I've addressed the error.

I've added removing the error every time the answer changes (no matter what widget it is) please play with it and let me know if you like it. The fact that there is a new answer doesn't mean there should be no error. We would need to validate constraints to check it but maybe it's better than assuming that the new answer is still wrong and removing the error only after navigating forward. So basically there is no perfect way but I think I like what I've implemented now a little bit more.

I really like what you implemented. I think this is more consistent with what the user would expect. Before, I kept thinking my new answer was wrong because the error message persisted. I think it makes more sense to validate the new answer once they try to move forward.

I also noticed you added the field text error message! It looks great :)

Small note: It looks like the bearing widget is still using the underline, instead of the new text field with the surface colour in the background.

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Sep 13, 2023

Thanks so much for trying this out! I prefer this option over the outline

Done.

Could we make the grey a bit lighter? Perhaps the same colour as the main menu buttons?

Done.

Small note: It looks like the bearing widget is still using the underline, instead of the new text field with the surface colour in the background.

Fixed.

@alyblenkin Do we need to change anything else?

@alyblenkin
Copy link
Collaborator

alyblenkin commented Sep 13, 2023

@alyblenkin Do we need to change anything else?

Not that I can think of! I think we are good to go :)

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Sep 14, 2023

Ok so two questions from me:

In the issue, you said:

Currently, the error fades in when you have a group of questions, which we want to keep inside the bounds of the outline. This might be something we want to pair on to get right.

are you ok with the fact that I removed that old animation? I think the outline is enough.

What do you think about displaying answers in widgets that should start an external app or use a sensor but it's not available and then we want to allow adding answers manually? It's always been handled using text fields but those text fields were disabled by default and their background were removed so that initially they looked like text views. I think we wanted that because if the external app existed we wanted the answer to look like in any other widget with buttons + text (eg. geo widgets, barcode widget etc.). But if the external app is not available we wanted to allow adding the answer manually and enable that text field. In this pr I didn't implement that first part (removing the background and making it look like a text view).
Screenshot_1694680607
Maybe it's something for a separate issue if we want it? I'm kind of torn.

@grzesiek2010
Copy link
Member Author

In audio widget (All widgets required form) the red error outline in shown after adding the audio.

Fixed.

Is this the expected look of the field to enter e.g strings?

Yes.

In Select with images the red outline is "covered" by images on the right.

In selects the line under choices goes beyond the red outline.

In Select Multiple widget with packed columns and no buttons, selected choice goes beyond the red outline.

This will be fixed in #5731 let's leave it for now as it is.

@grzesiek2010 grzesiek2010 merged commit a48ff5b into getodk:master Dec 7, 2023
6 checks passed
@srujner
Copy link

srujner commented Jan 10, 2024

Tested with Success!

Verified on device with Android 13

Verified cases:

  • Forms with required questions;
  • Forms with ReadOnly questions;
  • Light and Dark mode;
  • Minimize the app, rotate the screen;

@dbemke
Copy link

dbemke commented Jan 10, 2024

Tested with Success!

Verified on device with Android 10

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

Successfully merging this pull request may close these issues.

Fix spacing for read-only text widget Display errors inline rather than in toast
6 participants