Fix problem input [FC-0076] #1646
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1646 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 1100 1101 +1
Lines 21803 21858 +55
Branches 4711 4738 +27
=======================================
+ Hits 20330 20382 +52
- Misses 1401 1404 +3
Partials 72 72 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
👍 Working better than before, thank you for this @bradenmacdonald .
- I tested this using the PR test instructions
- I read through the code
- I checked for accessibility issues by using my keyboard alone to tab through a problem's answers and select text for inline formatting -- see note re inline image.
-
Includes documentationN/A -
User-facing strings are extracted for translationN/A
| * This could potentially cause problems if there are TinyMCE editors being used | ||
| * both on the parent page and inside a Paragon modal popup, but I don't think | ||
| * we have that situation. |
There was a problem hiding this comment.
Can this concern be resolved by passing editor.bodyElement to this function, and using the .closest() selector like you do below, instead of searching across the whole document?
To be clear, I didn't find any problems with using document in the problem editor, this is just a suggestion.
There was a problem hiding this comment.
I'm not sure, but I suspect that wouldn't help, because the various DIVs created to hold modals are always appended to the end of the body element and there's no way to distinguish between the ones that are for editors outside the modal and the ones that are for editors inside the modal.
What we really need is a TinyMCE API to get its modal DIV(s) given a reference to the editor object, but I looked at the entire API and the code and played around with the editor object in the JS console, and it doesn't seem to expose that publicly :/
| if (editor.bodyElement?.closest('.pgn__modal')) { | ||
| // This editor is inside a Paragon modal. Use this hack to avoid interference with TinyMCE's own modal popups: | ||
| reparentTinyMceModals(); | ||
| editor.on('selectionchange', reparentTinyMceModals); | ||
| } |
There was a problem hiding this comment.
The only editing issue I had here was when one of my problem's answers is an image:
image.as.answer.mp4
There's no way to select the embedded alt text to re-edit that image. But this was happening before, and is unrelated to this change.
There was a problem hiding this comment.
Weird. I wonder why it renders only the alt text and doesn't just show the (selectable) image. Would you mind opening that as a separate issue on this repo?
|
@ChrisChV could you please look at this PR as CC? I need a +1 from a CC to merge this. |
There was a problem hiding this comment.
@bradenmacdonald Looks good, but with this change emojis and formulas don't work in the text editor. I have tested it on master and it does work on that branch.
|
@ChrisChV Hmm, what browser are you using? They were working for me. I think I know what the problem might be, but I'm not sure how to reproduce it. |
Description
Fixes #1637 so that clicking editor toolbars doesn't try to close the modal.
Partially fixes #1636
Before:
After:
Supporting information
This "Insert" toolbar is shown in the "Expandable" text fields like Answer, Feedback, and Hint (all in the Problem editor), when you click on an empty line (or when the whole input is empty). Most people seemed fine with removing/shortening it, but I was asked on Slack to keep the "image" and "table" insertion tools. Which are also the ones in the official plugin docs for this floating toolbar plugin.
Testing instructions
Open a Problem component, both in a library and in a course. Set answers, feedback, and hints. Check that the formatting toolbars (bold/italic/etc. when you have text selected) and other editor modals (insert emoji, insert image, etc.) are clickable and working.
I also fixed a bug in the Studio header. Verify that clicking on the library name will take you back to the library home page:
Private ref: FAL-4037