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

Perseus final fixes #12362

Merged
merged 9 commits into from
Jul 3, 2024
Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jun 25, 2024

Summary

  • Does flyby fix of core logging module to allow multiple arguments to be sent to each logging method
  • Prevents restoring of slightly mangled state by santizing question keys before restoration
  • Fixes previously unnoticed issue whereby an initial refresh of the page would not restore the answer on first page load

References

Fixes #12322
Fixes #12375

Reviewer guidance

Use the database from referenced issue and see that the quiz responses are properly restored for all questions with no errors logged in the console.
While on the quiz page, refresh the page, and see that the answer gets properly filled in on page refresh.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Jun 25, 2024
@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. DEV: frontend and removed TODO: needs review Waiting for review labels Jun 25, 2024
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

I have a couple of comprehension questions but no real concerns, although I will leave manual QA to Peter :)

@pcenov
Copy link
Member

pcenov commented Jun 28, 2024

Hi @rtibbles, while fully regression testing I noticed just the following onscreen keyboard related issues:

  1. When I use several hints the keyboard gets displayed at the bottom of the screen which results in poor UX as the user has to scroll down and up. This is most noticeable in the Android app:
2024-06-28_16-23-46.mp4
  1. When viewing the lesson/report in less than about 800px, then the keyboard pops up when I move to the next question, or want to see the correct answer:
unneccessary.keyboard.mp4

Also I am not sure whether the issue described in #12322 sis fully fixed as I would expect that if the keyboard is present on screen then the input focus should be within the input field, which is still not the case:

2024-06-28_17-01-04.mp4

LogsAndDB.zip

@rtibbles
Copy link
Member Author

More troubling is you are still seeing the console errors that my fix was meant to prevent, I still haven't replicated these without your database directly, so I feel like I am missing something in the steps to replication here.

@pcenov
Copy link
Member

pcenov commented Jul 1, 2024

Hi @rtibbles - let me knows which scenario you can't replicate and I will try to add additional steps.

@rtibbles
Copy link
Member Author

rtibbles commented Jul 1, 2024

Hi @pcenov - the third video, I see the errors still occurring in the console as you switch between items, that's the scenario I am unable to replicate.

@pcenov
Copy link
Member

pcenov commented Jul 1, 2024

Hi @rtibbles my suspicion is that this is happening because in this quiz we have a mixture of questions requiring user input and a multiple select question. I was able to replicate this with the exact same quiz so here's the DB and logs in case you can make use of it:
LogsAndDB.zip
I'll test it again tomorrow with other quizzes.

@rtibbles
Copy link
Member Author

rtibbles commented Jul 1, 2024

Hi @pcenov, I was finally able to replicate the errors those last errors you were seeing, and I think I have resolved them all now.

To fix 1) I have changed the positioning of the keypad, so that it will now overlay any question content, rather than being pushed down. There didn't seem to be a happy medium between showing the content and having the keypad overlay, so I have gone with this.

For 2 - the keypad should no longer display on reports at all.

For 3 - the keypad should now dismiss when changing questions (I noticed it was persisting even when the next question did not have a numeric input).

@pcenov
Copy link
Member

pcenov commented Jul 2, 2024

Hi @rtibbles,

Unfortunately 1 is still not fixed - the keyboard is still showing throughout Coach > Reports and Coach > Lesson > Resources:

coach.mp4

Also perhaps and if possible we should have the keyboard always displayed at the bottom in the way you've currently implemented it when the exercise is part of a Lesson or viewed in the Library. Otherwise it's pretty bad when completing an assigned quiz, as it almost always covers the exercise:

Keyboard.covers.the.exercise.mp4

Here's how it looks in the Library page:

lesson.view.mp4

Also it should be possible to scroll the contents of the page while the keyboard is displayed.

Logs.zip

@rtibbles
Copy link
Member Author

rtibbles commented Jul 2, 2024

Hi @pcenov, I walked this through with @marcellamaki, and the most robust solution we could come up with is to default to the built in behaviour of perseus, for the keypad to attach to the bottom of the screen. I have updated the positioning of the element, so that it should always overlay any other element on the screen.

This does mean that while the keypad is open, the exercise bar and quiz navigation bar are obscured, but it was the only way to make it behave consistently across all contexts.

The display of the keypad when viewing reports should now also be fixed.

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Hi @rtibbles - I confirm that now the keypad is positioned at the bottom of the page as specified in your comment and that the user has to manually close it in order to click any of the buttons in the navigation bar.
The display of the keypad when viewing reports is also fixed.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

The last couple of commits look good as code changes, and manual QA approves ✅

@marcellamaki marcellamaki merged commit 05231d7 into learningequality:develop Jul 3, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc.
Projects
None yet
3 participants