-
Notifications
You must be signed in to change notification settings - Fork 636
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
Upgrade perseus to npm published version. #9759
Conversation
Build Artifacts
|
Hi @rtibbles, I was able to identify just the following issues by extensively testing various perseus exercises in multiple languages (valid in quiz, lesson or library context): The issues above can be replicated by importing Khan Academy (English - US curriculum) > Math by grade > 8th Grade > Illustrative Mathematics |
Thanks @pcenov - I assumed that there is no way this could have gone as smoothly as I thought it had! Will look into these issues, thanks for the precise reference for replication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM so long as it passes manual QA. Added comments for context are much appreciated (probably mostly by you some years later when you find the need to revendor Perseus).
// Keep a global register of all image URLs at the module level, | ||
// so that even if we have multiple instances of the PerseusRenderer | ||
// instantiated at once, we can still render images from all of them. | ||
// This also allows us to only monkey patch the Util functions once as | ||
// well, and prevent duelling components from overriding each other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multi-modal friendliness <3
Dismissing to block merge until manual QA finalized.
Hi @rtibbles, after testing the latest build the issue with the exponents and fractions disappearing is still valid while the one with the single/multiple choice questions is partially fixed as it is now possible to deselect a selected answer but there are some blue lines displayed each time I make a selection, a yellow border displayed if I deselect a selected answer and also if I navigate to another question after having deselected my answer when I go back to the same question the answer is selected again. Videos:
|
@rtibbles following a discussion in Slack I tested the reported issue with the missing bar graph and I can confirm that I am able to replicate it both at https://content-demo.learningequality.org/en/learn/#/topics/t/b18a118b93dc52fe9f1c68f9e418aaf5/search and when using the latest build from this PR. Here's also a video of what is being displayed in the latest build: 2023-01-06_17-41-13.mp4I also noticed an issue with one multiple-select question where the text for the number of answers that need to be selected is not displayed correctly: |
Hi @rtibbles - I confirm that the keyboard is displayed now and it works correctly when viewing an exercise from the Library page. However when you create a quiz and attempt to fill in an answer the keyboard is displayed too high and also doesn't fit the screen properly (at least on my Note 10 Samsung phone which has a very large screen) so that the Back and Close button are invisible to the user unless the user drags the keyboard to the left. See the video: quiz.view.mp4 |
Hi @pcenov I think I came up with a solution that seems to work well for both the exercise and quiz contexts, hopefully this is robust! |
Hi @rtibbles I confirm that now the keyboard fits better withing the screen in the Android app. Here's how it looks when compared to the previous implementation - might be worth investigating whether it can be further improved in a follow-up PR: Also when testing the .deb build if I shrink the browser width to less than 820px the on-screen keyboard pops up, which shouldn't be so, right: |
@pcenov this seems to have been a change by Khan Academy within perseus to display the keypad even on desktop - I think I'm ok with leaving it in, as it still allows use of the regular keys to enter the response. I think this may be a replacement for some of the previous tooltips they used to suggest permissable values (like pi, etc). |
Oh.. or maybe not, as although it pops up, it doesn't actually allow input via the keypad. |
OK, I have updated it so that the keypad does always show, regardless of modality, due to the lack of tooltips to offer prompts for how to answer, but in doing so, have also made sure that it works regardless of modality, and can still do keyboard input as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minimal feedback so far. I think reading commit by commit, the code is.... as comprehensible as it can be (most of the complexity related to our use of perseus in general, not the method of implementation)
line-height: 1.2; | ||
} | ||
|
||
table.periodic-table td.element div.atomic-num { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentionally empty? (it looks like one of the few places there were more recent modifications which is why I ask)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - the dist files are just directly copied from KA's npm packages, so 🤷 - I take no responsibility for these, except the minor manipulations we make to have valid font file URLs in them.
Upgrade to ServerItemRenderer API. Update message parsing and extraction.
Handle errors slightly more gracefully.
non-colliding recurrence of perseus rendering within the same page.
… mobile input. Update styling through the assessment stack to ensure we can position it properly.
Hi @rtibbles, Also spotted the following issues:
Ubuntu:
Windows:
2024-06-19_17-30-06.mp4Here's the error in the console:
tab.mp4 |
1 is definitely not related to the changes here, I have filed a follow up issue for it here: learningequality/morango#224 |
@pcenov I have fixed the inadvertent submission error you saw when dismissing the keypad, I was unable to replicate the other issue, and have filed a follow up here: #12322 if you could post the database from there so I can replicate, that would be helpful. The error that is occurring is due to the answer not matching to the question, so I am trying to work out how that has happened. |
re-testing of the mobile keyboard close input confirms that issue Peter noted is resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @rtibbles - with my last manual QA test, I've confirmed the remaining bug that Peter found is resolved
Summary
KatexProvider
andTeX
for handling rendering, borrowing the latter from the the deprecated Khan React componentsReviewer guidance
Test exercises in lots of different languages, and with lots of different exercise types! The most problematic are probably the more complex mathematical ones - graphs, lots of mathematical formulae etc.
Fixes #8362
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)