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

Hierarchy view styling #5885

Merged
merged 29 commits into from
Jan 17, 2024
Merged

Hierarchy view styling #5885

merged 29 commits into from
Jan 17, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Dec 22, 2023

Closes #5837

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

As discussed in the issue, the hierarchy view and margins used there should be consistent with what we have in form filling. To achieve that I used the same values defined in the dimen file.

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?

The pr is focused on UI improvements that shouldn't be risky for the functionality, therefore I think we can focus on the UI and not perform a lot of regression tests here. It contains some refactoring so it would be good to perform some of them though.

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

No.

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

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • 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 grzesiek2010 force-pushed the COLLECT-5837 branch 2 times, most recently from c30c014 to 4508dd6 Compare December 22, 2023 23:11
@grzesiek2010 grzesiek2010 marked this pull request as ready for review December 28, 2023 16:49
@alyblenkin
Copy link
Collaborator

@grzesiek2010 - thanks for fixing the build so I could review it! The hierarchy screen and form entry are feeling much more unified.

Notes:

  • Padding: looks much better, it doesn't feel as squished anymore.

  • Spacing between questions: now that I'm seeing it built out, I think we need to increase the spacing between questions just a little bit. I think this is making it harder to read when you have a longer form.

  • Folder icon: I'm quite pleased with the icon! I think it helps with navigation and establishing a hierarchy of information and my eye is drawn to the top first. I think we could add a bit more padding between the group name and question to establish more of a separation between the two.

  • Font size and weight: I think the question looks good, but the answer seems a bit too small. Can we increase it slightly? I'm also thinking we may want to change the text colour of the answer to blue to make it more obvious - figma. This could look terrible, but I'm curious to see if that helps.

  • Indenting answer: Indenting certainly helps with scanning the information when I compare it to what it looked like before. However, it's not working consistently across all question types. The answers for the required questions seem to appear aligned to the left, rather than indented (see below). Side note: Now that the answer is indented, it's very noticeable that when the required question falls onto two lines it doesn't get indented (so everything comes after the asterisk).

  • Table of contents / summary view: I don't think this should be indented because it makes it harder to read - I'm thinking that styling accidentally was applied there too. I also still think the label and name should be flipped, but we didn't prioritize it as part of this story. Maybe this is something we come back to.

Screenshot 2024-01-03 at 9 43 28 AM

@grzesiek2010
Copy link
Member Author

Indenting answer: Indenting certainly helps with scanning the information when I compare it to what it looked like before. However, it's not working consistently across all question types. The answers for the required questions seem to appear aligned to the left, rather than indented (see below). Side note: Now that the answer is indented, it's very noticeable that when the required question falls onto two lines it doesn't get indented (so everything comes after the asterisk).

The asterisk is a part of a question text. We don't have a separate view to display the asterisk we just display it in one TextView. Since it's one TextView if it consists of multiple lines they are not aligned vertically with the first line but with the asterisk that in fact is the beginning of the first line not a separate element as I said above. Do you think it would be better to have it in a separate element? Some time ago I wanted to refactor the way we display asterisks in form entry (they are displayed in the same way by adding them to question labels) but it turned out to be too complex. If you want I can check if it is possible to rework it in the hierarchy view at least.

Table of contents / summary view: I don't think this should be indented because it makes it harder to read

Do you mean that the Group text is indented too or maybe something else as well? I agree it shouldn't and maybe a good solution would be to display it above the question as you said https://www.figma.com/file/KFi9hIQdrRgo0rLqIC7Sg8/ODK-user-flows?type=design&node-id=3684-73031&mode=design&t=z5x1vMKVcsaifguG-0 to make it clear it's not the same as answers in other list elements

Spacing between questions: now that I'm seeing it built out, I think we need to increase the spacing between questions just a little bit. I think this is making it harder to read when you have a longer form.

Something like this would be better?

Before After
Screenshot_1704383830 Screenshot_1704384808

@alyblenkin
Copy link
Collaborator

The asterisk is a part of a question text. We don't have a separate view to display the asterisk we just display it in one TextView. Since it's one TextView if it consists of multiple lines they are not aligned vertically with the first line but with the asterisk that in fact is the beginning of the first line not a separate element as I said above. Do you think it would be better to have it in a separate element? Some time ago I wanted to refactor the way we display asterisks in form entry (they are displayed in the same way by adding them to question labels) but it turned out to be too complex. If you want I can check if it is possible to rework it in the hierarchy view at least.

That's helpful context. I think we can address this as a separate issue. I think it wasn't bad before, but more noticeable with the answer indented. I'll write up a story, and we can determine where it fits in the priority later :)

Do you mean that the Group text is indented too or maybe something else as well? I agree it shouldn't and maybe a good solution would be to display it above the question as you said https://www.figma.com/file/KFi9hIQdrRgo0rLqIC7Sg8/ODK-user-flows?type=design&node-id=3684-73031&mode=design&t=z5x1vMKVcsaifguG-0 to make it clear it's not the same as answers in other list elements

I haven't noticed it anywhere else, so just the group text for now.

Something like this would be better?

I think that's looking a lot better. The old spacing isn't so bad in this screenshot because the text is really short. The spacing becomes difficult when you have lengthy questions, making it harder to distinguish the separation between things. Do you have an example of when you drill down into the questions with the new spacing?

@grzesiek2010
Copy link
Member Author

I think that's looking a lot better. The old spacing isn't so bad in this screenshot because the text is really short. The spacing becomes difficult when you have lengthy questions, making it harder to distinguish the separation between things. Do you have an example of when you drill down into the questions with the new spacing?

I've just pushed that change so you can download the apk and play with it on your own. Please notice that now the top margin in the form entry will be smaller than the one in the hierarchy view and when you navigate from the form entry to the hierarchy view the question title goes down a little bit. I think we wanted to have it consistent so adjusting the margin in the form entry might be needed too?
ezgif com-video-to-gif-converter

@alyblenkin
Copy link
Collaborator

I've just pushed that change so you can download the apk and play with it on your own.

Thank you! The spacing works a lot better for longer questions as well. Is it possible to reduce the vertical padding just a little bit, so it isn't as drastic when you have shorter text?

Please notice that now the top margin in the form entry will be smaller than the one in the hierarchy view and when you navigate from the form entry to the hierarchy view the question title goes down a little bit. I think we wanted to have it consistent so adjusting the margin in the form entry might be needed too?

I agree, we need to make the top margin consistent so it doesn't jump around.

@grzesiek2010
Copy link
Member Author

Is it possible to reduce the vertical padding just a little bit, so it isn't as drastic when you have shorter text?

on master it was 8dp now it's 16dp so if you want something between we can try 12dp?

@alyblenkin
Copy link
Collaborator

on master it was 8dp now it's 16dp so if you want something between we can try 12dp?

Yes, something between 10dp or 12dp will work well. Let's try 12dp first. Let me know if it's easier to hop on a call to tweak it either today or Monday.

@grzesiek2010
Copy link
Member Author

Yes, something between 10dp or 12dp will work well. Let's try 12dp first. Let me know if it's easier to hop on a call to tweak it either today or Monday.

I've used 12dp

and also updated the text appearance used for displaying answers:

Before After
Screenshot_1704718509 Screenshot_1704718910

the last thing is that I've reworked the way list items for groups are displayed (moving the group label above the title) according to https://www.figma.com/file/KFi9hIQdrRgo0rLqIC7Sg8/ODK-user-flows?type=design&mode=design&t=kITBl6XtaOjSEUri-0

Please review.

Folder icon: I'm quite pleased with the icon! I think it helps with navigation and establishing a hierarchy of information and my eye is drawn to the top first. I think we could add a bit more padding between the group name and question to establish more of a separation between the two.

Do you still want to add more spacing?

@alyblenkin
Copy link
Collaborator

I've used 12dp
and also updated the text appearance used for displaying answers:

Thanks for making those changes! For the spacing, 12dp works well. It's a small change, but it looks much better than the older version, and we are not impacting the experience too much for smaller devices.

I like the blue text because it makes the answer stand out. I did question if anyone would mistake it for a link, but I doubt that would happen because it's not underlined. Let's quickly review these design updates on the team call tomorrow before we make the final decision.

the last thing is that I've reworked the way list items for groups are displayed (moving the group label above the title) according to https://www.figma.com/file/KFi9hIQdrRgo0rLqIC7Sg8/ODK-user-flows?type=design&mode=design&t=kITBl6XtaOjSEUri-0

Ah okay, I noted in my first comment that this is something we didn't want to prioritize right now, which is why I didn't include it in the 'dev ready' page that you linked, but I do think it works a lot better! We can discuss this tomorrow as well.

Folder icon: I'm quite pleased with the icon! I think it helps with navigation and establishing a hierarchy of information and my eye is drawn to the top first. I think we could add a bit more padding between the group name and question to establish more of a separation between the two.

Do you still want to add more spacing?

I think this might be okay now that we've spaced out the rest.

@alyblenkin
Copy link
Collaborator

@grzesiek2010 - I also just noticed that the form entry text field is really pushed down now - I don't think that was happening last week.

Screenshot_20240108-181654_ODK Collect

@alyblenkin
Copy link
Collaborator

Hey @grzesiek2010, so based on everyone's feedback we want to:

  • remove the blue text for the answers, but keep the indenting
  • keep the group label above, as you have it now
  • keep the spacing at 12pd
  • we need to make sure the styling for the answer in the hierarchy and form entry answer is the same

@lognaturel
Copy link
Member

we need to make sure the styling for the answer in the hierarchy and form entry answer is the same

I was talking about the styling for the question. The answer text is currently bigger when filling out the form than in the hierarchy screen and I think that’s ok.

@alyblenkin
Copy link
Collaborator

I was talking about the styling for the question. The answer text is currently bigger when filling out the form than in the hierarchy screen and I think that’s ok.

This makes more sense and maybe that's why I was confused because I think the question styling is the same right now, but maybe I'm wrong!

@grzesiek2010
Copy link
Member Author

I also just noticed that the form entry text field is really pushed down now - I don't think that was happening last week.

Fixed.

remove the blue text for the answers, but keep the indenting

Fixed.

@alyblenkin
Copy link
Collaborator

I also just noticed that the form entry text field is really pushed down now - I don't think that was happening last week.
Fixed.

remove the blue text for the answers, but keep the indenting
Fixed.

Nice! It looks like the "Go To Start" and "Go To End" buttons are now touching. I'm not sure if that was happening before?

@grzesiek2010
Copy link
Member Author

It looks like the "Go To Start" and "Go To End" buttons are now touching. I'm not sure if that was happening before?

Ah yeah my mistake, it should be fixed now.

Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

@seadowg seadowg merged commit 0b92e73 into getodk:master Jan 17, 2024
6 checks passed
@dbemke
Copy link

dbemke commented Jan 17, 2024

What's the default settings when there are many question in a group and I go to the hierarchy when I'm e.g on a 6th question in the group. What should be at the top of the hierarchy view after going to it from 6th question of a group?
hierarchy

@grzesiek2010
Copy link
Member Author

If there are multiple questions in one group when you open the hierarchy view the list should be scrolled to display the question you were on at the top of the list if it's possible so:

  • if you open the hierarchy from the first question nothing should change
  • if you open the list from the second question, the list should be scrolled so that it's at the top (the same with all the other questions)
  • if you are on one of the last questions we just scroll the list to the bottom.

It has been like that, it's not a new behavior.

@grzesiek2010
Copy link
Member Author

The only problem I've just noticed is that during scrolling such a long list of question there is no margin between the group name and the list:
image

It's not a big deal we can file it as a separate issue.

@dbemke
Copy link

dbemke commented Jan 19, 2024

@alyblenkin Could you have a look if the indentation in groups/repeats, questions and answers are ok? It seems as expected to me but it would be great if you could confirm and then we would also have a verified example that we could add to our tests :)
gruopsRepeats

@dbemke
Copy link

dbemke commented Jan 19, 2024

I'm not sure how a new line in the answer field should be displayed. This is an example of the hierarchy view with answers containing words with a new line between - the space between lines seems bigger than the space between the question and the answer.
NewLineHierarchy

@alyblenkin
Copy link
Collaborator

@alyblenkin Could you have a look if the indentation in groups/repeats, questions and answers are ok? It seems as expected to me but it would be great if you could confirm and then we would also have a verified example that we could add to our tests :)

@grzesiek2010 @seadowg feel free to jump in if I'm wrong, but this looks like the expected behaviour.

I'm not sure how a new line in the answer field should be displayed. This is an example of the hierarchy view with answers containing words with a new line between - the space between lines seems bigger than the space between the question and the answer.

@dbemke, you have such a great eye for this kind of detail 😍 I agree with you that the spacing should be the same. However, I don't think this is a show-stopper! If it is an easy fix, I think we should match the spacing between the question and answer (this might be a bit tight, but it's hard to say until we see it).

@grzesiek2010
Copy link
Member Author

you have such a great eye for this kind of detail 😍 I agree with you that the spacing should be the same. However, I don't think this is a show-stopper! If it is an easy fix, I think we should match the spacing between the question and answer (this might be a bit tight, but it's hard to say until we see it).

Please file separate issues for spacing and the margin between the group name and the list. Everything else seems to be fine.

@dbemke
Copy link

dbemke commented Jan 22, 2024

Tested with Success!

Verified on devices with Android: 8.1, 10

Verified cases:

  • hierarchy view of questions, answers, groups, repeats
  • hierarchy view in light and dark mode
  • landscape and portrait view
  • scrolling the hierarchy view
  • spacing, margins in the hierarchy view
  • issues found in the PR will be filed separately

@srujner
Copy link

srujner commented Jan 22, 2024

Tested with Success!

Verified on devices with Android: 13

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.

Hierarchy view styling
6 participants