Fix/checkpoint copy Paced Social Annotation: update Checkpoint copy and skip the due-date step. - #7433
Merged
Merged
Conversation
Match the wording design asked for: - "Manual" becomes "Manual reveal", with a subtitle explaining that the instructor clicks a button to reveal student annotations. The subtitle is rendered by `RadioGroup.Radio`, wrapped in `small` like the other radio subtitles in the file picker (see `AutoGradingConfigurator`). - "More coming soon" becomes "More options coming". - The note below the radios now defines what a Checkpoint is instead of describing the manual reveal. The new note holds for any checkpoint type, so it no longer hangs off `showManualNote`. That condition was already dead — `CheckpointType` has a single member, so it never evaluated false, which is why it needed an eslint-disable for `no-unnecessary-condition`. No style changes: the copy is the only thing that moves.
The date the step collects has no effect yet, so offering it suggests the assignment does something it doesn't. Michael asked for it to be hidden until due dates are wired up. "Next" on the checkpoint step now completes the workflow instead of advancing to the due date, which leaves the step unreachable. Nothing else was removed: the state, the ref, the selector, its title and the `'due-date'` member of the step union all stay, and the original transition sits commented out right above the temporary one. Restoring it, and the blocks commented out in the test, brings the step back. The now-unreachable validation guard gets an `istanbul ignore` so its `return` doesn't break the 100% statement threshold. `DueDateSelector` and its tests are untouched.
Comment on lines
+237
to
+238
| // Regular flow takes over: the due-date step that used to sit here is | ||
| // skipped for now (see `goToNextWorkflowStep`). |
Contributor
There was a problem hiding this comment.
Why did you remove these two lines from the tests, while commenting them out in the other cases?
If you're going to comment out all the tests, please comment out these lines as well.
Comment on lines
-308
to
-311
| clickNext(wrapper); // -> due-date | ||
| assert.isTrue(wrapper.exists('DueDateSelector')); | ||
|
|
||
| clickBack(wrapper); // -> checkpoint |
Contributor
There was a problem hiding this comment.
Why did you remove these lines instead of commenting them out?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update the Checkpoint step copy
Matches the wording design asked for:
RadioGroup.Radio's ownsubtitleprop and wrapped insmall, the same way the other radio subtitles in the file picker are done (seeAutoGradingConfigurator) — no new CSS.Since the new note holds for any checkpoint type, it no longer hangs off
showManualNote. That condition was already dead code:CheckpointTypehas a single member, so it never evaluated false — which is why it carried an eslint-disable forno-unnecessary-condition.Styles are untouched; this is copy only.
Skip the due-date step for now
The date the step collects has no effect yet, so showing it suggests the assignment does something it doesn't. Hidden until due dates are actually wired up.
"Next" on the checkpoint step now completes the workflow rather than advancing to the due date, which leaves that step unreachable. Nothing was deleted: the state, the ref,
DueDateSelectorand its render, the card title and the'due-date'member of the step union all stay, and the original transition sits commented out directly above the temporary one. Restoring it — plus the blocks commented out inFilePickerApp-test— brings the step back.The validation guard that is now unreachable gets an
istanbul ignoreso itsreturndoesn't break the repo's 100% statement threshold.DueDateSelectorand its 26 tests are untouched.Effect
Instructor picks Paced Social Annotation → Checkpoint → Next goes straight to content selection. The payload still sends
due_date: null, exactly as it does today when the field is left blank, so the backend sees no change.