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

Random spacing issues and error messages in Renderer #438

Open
huip-ims opened this issue May 20, 2024 · 12 comments
Open

Random spacing issues and error messages in Renderer #438

huip-ims opened this issue May 20, 2024 · 12 comments
Assignees

Comments

@huip-ims
Copy link
Collaborator

@anthonypetersen Hi Tony, We were testing the Module 2 Spanish markdown in the renderer on Friday and today, and we noticed some random spacing issues and error messages popping up.

In the first image below, you can see the following:

  • Text boxes now have "Example: …" in gray. However, the 2nd text box has "Example: sum(isDefined". Is this normal?
  • Also, some line breaks are not recognized thus causing random spacing issues which weren't present before (i.e, "O, si le es más fácil recordar en qué año, anótelo aquí:" is now not on a separate line.).
  • I'm also seeing random error messages now popping up as shown in red text in the same image, so it looks like some functions that worked previously are now not working. Do you know what may be causing these issues?

In the 2nd image below, it looks like grid row & column headers are now in bold. Also, in some grids, the selection buttons on each row are not aligned. Are these normal and recently added?

Currently, we cannot move forward with our markdown testing in the renderer until these issues are resolved. Could you please help us look into these issues in the renderer?

Thanks,
Pete

random_issues

Grid_bold_column_row_headers

@anthonypetersen
Copy link
Contributor

@huip-ims can you please provide the markdown you are testing with?

@huip-ims huip-ims self-assigned this May 21, 2024
@huip-ims
Copy link
Collaborator Author

@anthonypetersen Tony - attached is the Spanish Module 2 markdown in testing.
module2_ES.txt

@danielruss
Copy link
Member

image

This table (though not the same) looks good in chrome, does it look the same for you? What browser are you using?

@huip-ims
Copy link
Collaborator Author

@danielruss @anthonypetersen Daniel - We're using Chrome as well. In your screenshot, although the selection buttons on each row are aligned, the column headers are overwriting each other. In ours, the column headers display correctly, but the selection buttons are not.

image

@danielruss
Copy link
Member

@JoeArmani I cannot exactly reproduce this error. I want to say this is related to the table view. Have you seen any cases where this happens. Maybe we could add some CSS that centers the text vertically in a TD (if there is not a center-vertically, look into margin-top: auto; margin-bottom: auto.

Also, quest uses bootstrap, so we use bootstrap table https://getbootstrap.com/docs/5.0/content/tables/#vertical-alignment

let me know what you think.

@JoeArmani
Copy link
Contributor

@danielruss I agree; it's related to the table view. I have not seen the misaligned vertical spacing, but Aileen and I found an issue with the rendering of this Module 2 table yesterday (this same 11-column table). The click targets aren't all accessible on a narrower screen for a screen reader, either. I'm actively looking into this.

@danielruss
Copy link
Member

Let me know if you any help from me.

@JoeArmani
Copy link
Contributor

Hi @danielruss, I have a PR in to address:

  • Responsive sizing for the grids.
  • Improved accessible targeting for the grids.
  • Grid misalignment in renderer should be fixed.
  • Handle placeholder for all isDefined min & max numeric values.

I'm unclear on the errors in the renderer. Here's what I found:
I tested the IMS Spanish code on the current branch and backtested it on earlier commits on the main branch with what appearred to be the same results each time (I was trying to see if something had broken in the process).
Tested the current one
This one: 3e0b2bf
And this one: d459d25
•Results appearred to be the same each time with the sum(isDefined) error.
•Then I found the sum(isDefined) function usage in module1, tested it, and it worked well.
•I traced through evaluateCondition() in the renderer, but I’m not familiar with that part of the app, so I’m not sure what we’re looking for. Thanks for any info you can provide.

Just FYI, I’m OOO on a site visit today but I’ll be back tomorrow.
Screenshot 2024-05-21 at 2 50 44 PM

@JoeArmani
Copy link
Contributor

JoeArmani commented May 23, 2024

I was able to load this module2_ES.txt survey into the PWA. Here's a side-by-side logging comparison with additional logs added.

Here's what's happening:
In the PWA, we have context from the previous results object
Screenshot 2024-05-23 at 2 15 13 PM

We don't have this context in the renderer. When replaceValue gets called, it looks to those previous results to swap out the variable for the value
Screenshot 2024-05-23 at 2 18 10 PM

Here's a side-by-side comparison of the process (PWA on the left, with context and renderer on the right without context)
Screenshot 2024-05-23 at 2 12 27 PM

So, to make this work in the renderer, we could supply default values for those variables that are passed in when context exists.
Here are the context values we set in the PWA:
firstName age AGE ...other values are CIDs for module numbers (not important in the renderer IMO).

@danielruss
Copy link
Member

danielruss commented May 23, 2024

evaluateCondition handles runtime calculations that cannot be handled while parsing, because it may depend on values that are not yet entered. The line you are pointing to is an old fix to a problem where the answer to a question is the actual number 0. How many children do you have? 0. The zero is falsy and causes problems.

This whole section of code needs to be re-written. Somewhere we started using data-* (think min/max) and we had a mix of data-* and just *. Massive confusion.

The error you see in the dev panel is cause by not defining AGE. Go the the settings and in the "previousValues" add the following JSON:

{"AGE":"45"}. It should work

@JoeArmani
Copy link
Contributor

Thanks for the heads up on settings. I added that and the validation seems to do what's expected. Am I missing something here or is it possible the {"AGE":"45"} setting wasn't added when this was being tested?

Screenshots from renderer in my current working branch...

Screenshot 2024-05-23 at 4 26 02 PM Screenshot 2024-05-23 at 4 24 09 PM

@danielruss
Copy link
Member

The development tool does not know about Connect, so it does not get previous values from Firestore. You need to manually add the values. However, the previous results you added are stored in indexedDB, so you wont have to enter it again. (It's actually a mixed blessing).

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

No branches or pull requests

6 participants