Skip to content

Conversation

@chelaz1234
Copy link

@chelaz1234 chelaz1234 commented Aug 28, 2025

Any italic text should be deleted from the final Pull Request text, including this line

Description

This component is allows students to review their selected tasks and learning summary report before final submission. It checks for required components like task selection and ILO alignment, displays warnings if anything is missing, and lets students confirm their portfolio is ready for assessment._

Type of change

  • Migrated: portfolio-review-step.coffee →portfolio-review-step.component.ts, .html, .scss

  • Deleted: portfolio-review-step.coffee,portfolio-review-step.tpl.html

  • Updated: doubtfire-angularjs.module.ts (removed old directive reference)

  • Updated: portfolio.tpl.html to mount the new Angular 17 component

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Before:
portfiloio ss

After:
Screenshot 2025-09-11 200938
449666951-542a0b80-550f-4432-ae4d-68d3fb882dc5

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from @macite and @jakerenzella on the Pull Request

Copy link

@Triet-coder Triet-coder left a comment

Choose a reason for hiding this comment

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

Screenshot 2025-09-21 143210 Screenshot 2025-09-21 143114 Solid migration — review step validates task selection + ILO, shows clear warnings, and the confirm flow is clean. UI feels responsive; no regressions. Tiny nits: add a test for missing-state edge cases and disable Confirm until all checks pass.

@EkamBhullar
Copy link

Looks great. I have tested this migration, and it works perfectly fine. You can just try including some screenshots, so it is easy to compare

@chelaz1234
Copy link
Author

Hi @EkamBhullar, I’ve added screenshots showing the before and after states of the migration. You can now compare them side by side.

@chelaz1234
Copy link
Author

Screenshot 2025-09-21 143210 Screenshot 2025-09-21 143114 Solid migration — review step validates task selection + ILO, shows clear warnings, and the confirm flow is clean. UI feels responsive; no regressions. Tiny nits: add a test for missing-state edge cases and disable Confirm until all checks pass.

hanks for the detailed review!
I’ve fixed the suggestions you mentioned — added a test for the missing-state edge cases and updated the flow to keep the Confirm button disabled until all checks pass.

Copy link

@HasinduWelarathne HasinduWelarathne left a comment

Choose a reason for hiding this comment

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

Tested locally. All the functions behaves correctly , routing/deep links OK, no regressions observed. Code looks clean. LGTM
Capture after migration

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants