Trial displaying inequality seed in input previews#1889
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1889 +/- ##
==========================================
- Coverage 43.29% 43.19% -0.10%
==========================================
Files 575 575
Lines 24688 24747 +59
Branches 7312 8188 +876
==========================================
+ Hits 10688 10689 +1
+ Misses 13951 13382 -569
- Partials 49 676 +627 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…lity-seed-previews [VRT] Update baselines for feature/display-inequality-seed-previews
…equality-seed-previews
|
The VRTs haven't run. This might be because the base isn't main yet? I am expecting the Inequality ones to have changed, so keep an eye out when it gets switched. |
…lity-seed-previews [VRT] Update baselines for feature/display-inequality-seed-previews
| />} | ||
| {!readonly && <div className="eqn-editor-input"> | ||
| <div ref={hiddenEditorRef} className="equation-editor-text-entry" style={{height: 0, overflow: "hidden", visibility: "hidden"}} /> | ||
| {previewText && <i className="text-muted small">Click in either box below to edit your answer.</i>} |
There was a problem hiding this comment.
Since the boxes are in a different order for logic questions, this is in the wrong place. It should be moved to ~L220 to be above both.
| placeholder="Type your formula here" className={classNames({"h-100": isPhy}, {"text-body-tertiary": emptySubmission})} | ||
| /> | ||
| {initialSeedText && <button type="button" className="eqn-editor-reset-text-input" aria-label={"Reset to initial value"} onClick={() => { | ||
| updateEquation(''); |
There was a problem hiding this comment.
Resetting the text input without also resetting the Inequality input causes some strange desyncing and unexpected behaviour.
The current behaviour (which turns out to be two different errors colliding) on a Chemistry question with showInequalitySeed is the following:
- The text input and symbolic preview will update to suggest that e.g. "He ->" is available as a seed
- Clicking on the preview to open Inequality will not show it on the canvas
- Closing Inequality will display "He ->" now in black (but not in the text input)
- Clicking "Check my answer" will respond that the user did not provide an answer.
This is clearly wrong.
Firstly, resetting the equation should also reset the seed on the canvas (so that it re-appears and matches the preview). I think this can happen fairly simply in the same way as it does for the reset button on the modal, since the cleanup after this already handles any side effects:
| updateEquation(''); | |
| updateEquation(''); | |
| if (sketchRef.current) sketchRef.current.loadTestCase(editorSeed ?? ""); |
There was a problem hiding this comment.
The second issue is more complicated and needs a more widespread fix. It's mainly to do with how previewText is calculated on SymbolicChemistry and SymbolicLogic questions, along with how old question attempts are displayed on a new page load.
I wasn't sure how we should handle this, so I've put some changes together in #1988 that I think deal with the whole problem, but I'm open to suggestions on the specifics.
Essentially, previewText was relying on currentAttemptValue.result having a value to determine whether or not to show the seed, but this doesn't work for cases where the modal is entered but left empty, nor for cases where there is a previous attempt but the previous attempt was blank.
In order to track both of these, I add a modalRecentlyOpened state which is false on an unattempted questions/when the reset button has been used, and true on attempted questions/when the modal has been used. This explicitly controls whether the seed preview should be displayed or not (On reflection, perhaps it should be called displaySeed or similar to accommodate all cases).
The other part is the way the textInput is initialised. We used to always set it to "" and then in a useEffect set it to the computed expression on first load. With seeds, we can no longer compare against "" to check for first load, and this was quite messy anyway, so I suggest moving textInput to be computed after currentAttemptValue and (where applicable) using this value directly on first load instead. This also helps with loading blank previous attempts, as they otherwise had a seed displaying where they shouldn't.
There was a problem hiding this comment.
I've been playing around with these changes and they seem great, thanks for looking at it. My one "concern" with the approach is that I'm not sure whether it makes sense to allow submission of an empty answer in the first place – we don't allow this for other question types like numerics or string match (then again, we do for text entry..? but this is mostly unused...). Disallowing them would have prevented the issues with previewText relying on a potentially empty / blank currentAttemptValue.result, making for slightly simpler logic (though I suppose old attempts to these qs would remain blank, even if we changed this...).
I am not sure whether you would prefer the consistency between question types or the more capable approach we have here; I don't mind, there's no harm in keeping the work you've done here as is. Did you have any thoughts?
On reflection, perhaps it should be called displaySeed or similar to accommodate all cases
Agreed, coming into this without much initial perspective, this name helped explain what it did a little better.
There was a problem hiding this comment.
I don't see any use-case for allowing empty submissions so I'm all for disallowing them from now on, but these questions are widespread enough that I'm sure there are plenty of old empty currentAttemptValue.results around and I'd rather they appear consistently - especially since they'll still have "You did not provide an answer." feedback displayed (and we should certainly still show that they've made an attempt somehow!).
I think most of my work is applicable either way, but the specific part of supporting when currentAttemptValue.result is empty is also important both as a failsafe for new attempts, and because I'm not sure how else we'd handle it for these old ones.
Cool, let's go with displaySeed. I trust your ability to change it 🙏
…-see-previews-suggestions Inequality Seed Preview Suggestions
…lity-seed-previews [VRT] Update baselines for feature/display-inequality-seed-previews
Note that this requires feature/standardise-small-tag-usages, as between these the
<small>tags in the Inequality preview have been normalised.Enables previewing of the seed in Inequality input boxes (both typed and interfaced), before interacting with Inequality in any way. Whether or not a seed is displayed depends on, of course, whether there is a seed in the first place, and then the type of Inequality question:
A seed must also be greyed out if it has not yet been modified by the user. Note that content questioned whether it could grey out on click, but this doesn't count as an attempt so was inconsistent with when the button to submit enabled.
As requested, there is now also a reset button that resets an answer to the seed. This resets the colour to grey, and the answer to non-submittable.