Skip to content

feat(TimedSteps): New type of step containing a series of timed components #2198

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

feat(TimedSteps): New type of step containing a series of timed components #2198

wants to merge 12 commits into from

Conversation

Aaron-Detre
Copy link
Contributor

@Aaron-Detre Aaron-Detre commented Jun 25, 2025 •
edited
Loading

Changes

  • Added authoring for timed steps on the node authoring page.
  • Steps marked as timed display a series of components one at a time, each with its own countdown timer.
  • When the timer reaches 0 or the proceed button is clicked, that component is hidden and the next one is made visible.
  • When all components have been completed, a message saying the step has been completed is displayed.
  • When working on a timed step, navigation to other steps using the step toolbar is disabled. (You could still edit the URL in theory)

Test

  • Make sure everything works as described above.
  • If you have viewed or worked on a timed component, and then you close the tab and reopen it, you should see the next component instead of the one you were looking at before.
  • Components with no time limit specified and negative time limits are treated the same as a time limit of 0 and basically skipped.
  • Previewing should function the same as working on a timed step normally, except when you return to the timed step after completing it, you should start from the beginning rather than seeing the step completed message.

Other

  • I'm not currently saving time spent on each component, so that might be something to add later. I guess you would just store how long it takes them to press the proceed button if they press it at all? It wouldn't necessarily tell you how long it took them to work on the component, since they could finish it and then wait for time to run out. I guess it would be on the unit author to make it clear that they should click proceed as soon as they're done working.
  • I think originally we were talking about proceeding when the student clicks submit on the component, but I'm not sure it would be clear that that is what's going to happen when they click submit, and not all component types have a submit button. I think that a dedicated proceed button right next to the timer makes it pretty obvious, but it does make knowing the actual time spent on the component a little trickier.

@Aaron-Detre Aaron-Detre self-assigned this Jun 25, 2025
@Aaron-Detre Aaron-Detre added the enhancement New feature of any size or improvement (UI, performance, security) label Jun 25, 2025
@Aaron-Detre Aaron-Detre marked this pull request as ready for review June 25, 2025 20:27
hirokiterashima
hirokiterashima requested changes Jun 26, 2025
View reviewed changes
Copy link
Member

@hirokiterashima hirokiterashima left a comment

Choose a reason for hiding this comment

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

Nice job! 👍 The functionality mostly seems to work as you described. I agree that the having the "proceed" button next to the timer makes it clearer for the user.

I noticed some minor issues (below) and code improvement suggestions (inline).

Issues, improvements

  • See code improvement suggestions inline
  • In preview, I finish the step and see the "You have completed this step” message. I then go to previous step and come back, and it lets me work on the first component again.
  • Countdown glitch. Sometimes, due to some combination of letting the component timer finish and clicking on "proceed" for others, the timer seems to decrease erratically, like in 2-second decrements. See this video:
timer_glitch.mov

Questions

  • Should we default to 60(?) seconds for each timed component in the authoring tool? We should require a non-null number for the timeLimit value to simplify code. Having a default can help ensure that.
  • Right now, the timeLimit input is always shown for all components in the step in the. authoring tool. Should we only show it when the component is being edited? In this scenario, it would be good to show the time limit value for each component.

src/assets/wise5/themes/default/themeComponents/stepTools/step-tools.component.ts
Comment on lines +143 to +147
if (this.timedStep) {
return !this.timedStepCompleted;
} else {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Write in one line?

src/assets/wise5/themes/default/themeComponents/stepTools/step-tools.component.ts
@@ -43,14 +44,17 @@ export class StepToolsComponent implements OnInit {
protected nodeStatus: any;
protected nodeStatuses: any;
protected prevId: string;
@Input() private timedStep: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making timedStep an input to this component, can we set this in updateModel()?

Suggested change
@Input() private timedStep: boolean;
private timedStep: boolean;

src/assets/wise5/vle/vle.component.html
@@ -26,7 +26,7 @@

<ng-template #defaultVLETemplate>
@if (layoutState === 'node') {
<step-tools class="control-bg-bg mat-elevation-z1" />
<step-tools class="control-bg-bg mat-elevation-z1" [timedStep]="isCurrentNodeTimed()" />
Copy link
Member

Choose a reason for hiding this comment

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

Calculate timedStep boolean in StepToolsComponent.updateModel()? Remove isCurrentNodeTimed() function?

Suggested change
<step-tools class="control-bg-bg mat-elevation-z1" [timedStep]="isCurrentNodeTimed()" />
<step-tools class="control-bg-bg mat-elevation-z1" />

src/assets/wise5/authoringTool/node/node-authoring/node-authoring.component.ts
Comment on lines +224 to +225
const node = this.projectService.getNodeById(this.node.id);
return node.timed !== null && node.timed;
Copy link
Member

Choose a reason for hiding this comment

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

Simplify?

Suggested change
const node = this.projectService.getNodeById(this.node.id);
return node.timed !== null && node.timed;
return this.projectService.getNodeById(this.node.id).timed;

src/assets/wise5/vle/timed-node/timed-node.component.ts
export class TimedNodeComponent extends NodeComponent {
private componentTimers: number[];
private currentComponentIndex = 0;
private currentInterval: any;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private currentInterval: any;
private currentInterval: number;

src/assets/wise5/vle/timed-node/timed-node.component.ts
return this.components.at(this.currentComponentIndex).id;
}

private getComponentSubmitArgs(componentId: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private getComponentSubmitArgs(componentId: string) {
private getComponentSubmitArgs(componentId: string): any {

src/assets/wise5/vle/timed-node/timed-node.component.ts
}
}

private saveUnsavedWork() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private saveUnsavedWork() {
private saveUnsavedWork(): void {

src/assets/wise5/vle/timed-node/timed-node.component.ts
}

private saveUnsavedWork() {
if (!this.isPreview()) {
Copy link
Member

Choose a reason for hiding this comment

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

Check !isPreview() before calling this function?

src/assets/wise5/vle/timed-node/timed-node.component.ts
Comment on lines +77 to +79
this.components.forEach((component, index) => {
this.componentToVisible[component.id] = index === this.currentComponentIndex;
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we call this.hideComponents() here?

Suggested change
this.components.forEach((component, index) => {
this.componentToVisible[component.id] = index === this.currentComponentIndex;
});
this.components.forEach((component, index) => {
this.componentToVisible[component.id] = index === this.currentComponentIndex;
});

src/assets/wise5/vle/timed-node/timed-node.component.ts
}

protected updateComponentVisibility(): void {
return;
Copy link
Member

@hirokiterashima hirokiterashima Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It'd be good to have a comment about why we're overloading it with an empty body.

Suggested change
return;
// do nothing because ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hirokiterashima hirokiterashima hirokiterashima requested changes

@breity breity Awaiting requested review from breity

Assignees

@Aaron-Detre Aaron-Detre

Labels
enhancement New feature of any size or improvement (UI, performance, security)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Aaron-Detre @hirokiterashima