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

fix(#8541): fixes display of Enketo select option #8565

Merged
merged 9 commits into from
Oct 26, 2023

Conversation

latin-panda
Copy link
Contributor

@latin-panda latin-panda commented Sep 20, 2023

Description

  • Fixes display for:
    • Long and shot words
    • Long and short sentences

#8541

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@latin-panda
Copy link
Contributor Author

@garethbowen can you please review this quick fix? Thanks

@garethbowen
Copy link
Member

It looks like the width has broken radio buttons:

image

Have you got a form with a select that you can share so I can test this out?

@latin-panda
Copy link
Contributor Author

@garethbowen I used the enketo widget form

@latin-panda latin-panda removed the request for review from garethbowen September 21, 2023 06:13
@latin-panda
Copy link
Contributor Author

@garethbowen please hold, I'm checking some input display

@garethbowen
Copy link
Member

@latin-panda How are you getting on with this one?

@latin-panda
Copy link
Contributor Author

It turned out to be more challenging to fix than I expected, but I think it's okay now.

Testing using enketo widget form

Short text
Long text
Just normal text

@garethbowen can you please review?

@tatilepizs
Copy link
Contributor

Hi @latin-panda,

I was testing your branch and as you said, everything is looking fine with the enketo_widgets form, but the other forms that contain translations display all the available labels. Could you please check what might be wrong?

8541_long_names_enketo

image
image

master

image
image

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Overall looks good, but rejected based on the bug.

@latin-panda
Copy link
Contributor Author

@tatilepizs I've fixed it.
Inputs with dropdowns and long text behave like in master branch (no wrapping text), but the original issue is solved. The reason is that I don't have enough uniqueness (classes, ids, etc) in the Enketo's HTML to isolate the change without affecting other fields.

Screenshots Screenshot 2023-10-24 at 5 02 41 pm Screenshot 2023-10-24 at 5 02 15 pm Screenshot 2023-10-24 at 5 02 56 pm Screenshot 2023-10-24 at 8 43 11 pm Screenshot 2023-10-24 at 8 42 53 pm Screenshot 2023-10-24 at 8 42 36 pm

Copy link
Contributor

@tatilepizs tatilepizs left a comment

Choose a reason for hiding this comment

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

Thank you @latin-panda for fixing it so quickly ✨

I just have a question that is definitely not a blocker.
Why for some elements it is going to the next line in the correct place (blank space) and in other elements is cutting the word?
Is this something related to the code or is more related to the form? Do you know?

For example:

Places when the sentence/word is cut in the correct way:

image
image
image

Places when the sentence/word is cut in the wrong way:

image
image
image

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Starting to look really good, thanks!

I've left a few suggestions inline.

Also in your screenshot here the dropdown option label isn't wrapping at all. I didn't manage to figure out what was going on but maybe you can see something?

https://user-images.githubusercontent.com/66472237/277677449-dad4ca04-b6e0-47dc-96ed-3848e0fbd53d.png

Comment on lines 34 to 35
margin: 0;
display: contents;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think display: contents is valid in this context. I get quite a nice result if I remove these two lines altogether...

Suggested change
margin: 0;
display: contents;

Your PR:

Screenshot from 2023-10-25 12-19-12

My suggestion:

Screenshot from 2023-10-25 12-19-02

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the display: contents; so radios and checkboxes utilize the space efficiently and display nicely when in columns.
This is without display: contents;
Screenshot 2023-10-25 at 11 50 24 am
This is with display: contents;
Screenshot 2023-10-25 at 11 52 40 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the margin

Copy link
Member

Choose a reason for hiding this comment

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

Ahh ok, thanks.

webapp/src/css/enketo/medic.less Outdated Show resolved Hide resolved
webapp/src/css/enketo/medic.less Outdated Show resolved Hide resolved
@latin-panda
Copy link
Contributor Author

latin-panda commented Oct 25, 2023

Also in your screenshot here the dropdown option label isn't wrapping at all. I didn't manage to figure out what was going on but maybe you can see something?

@garethbowen That's what I tried to explain here, I didn't find a way to wrap it, and that doesn't affect other fields display

Edit:
I kept trying to fix it this morning and afternoon but no luck.
For the record, I tried white-space: break-spaces; but it wraps the word weirdly. It doesn't work either:
Screenshot 2023-10-25 at 1 33 17 pm

@latin-panda
Copy link
Contributor Author

latin-panda commented Oct 25, 2023

Why for some elements it is going to the next line in the correct place (blank space) and in other elements is it cutting the word? Is this something related to the code or is more related to the form? Do you know?

@tatilepizs somewhere else in the code, there's an additional rule to break by word instead of by letter when the field is in a column. I initially made it by letter in case of weird big words, which may be uncommon. It looks like the browser deals with long words just well, so because of that unlikely scenario, I changed it to break by word that looks nicer. Thanks for the observation!

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Thanks for going back and forwards on this!

@garethbowen
Copy link
Member

This is so complicated and requires so much testing I wonder if we need a test suite which compares screenshots. Then we can create a form with all of these edge cases and automatically check that we haven't regressed somewhere else...

Copy link
Contributor

@tatilepizs tatilepizs left a comment

Choose a reason for hiding this comment

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

It looks so much better @latin-panda, thanks ✨

Images attached

image
image
image
image
image
image
image

@latin-panda latin-panda merged commit a2d0cca into master Oct 26, 2023
34 checks passed
@latin-panda latin-panda deleted the 8541_long_names_enketo branch October 26, 2023 02:02
@dianabarsan
Copy link
Member

dianabarsan commented Oct 26, 2023

This change ended up making these radio span label elements not interact able in new wdio, and also in chrome dev tools - if I open dev tools and click on an element, it won't get focused :(

Benmuiruri pushed a commit that referenced this pull request Oct 26, 2023
Fixes display for:
- Long and shot words
- Long and short sentences
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

4 participants