Skip to content
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.

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

Pipd 945 show pop up once the wizard is complete #619

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

AdityaMantripragada
Copy link
Collaborator

No description provided.

@AdityaMantripragada AdityaMantripragada added the DO NOT MERGE Definitely don't merge this PR! label Oct 31, 2024
@AdityaMantripragada AdityaMantripragada added Ready For Review PR is code-complete (or very close) and only needs some review and/or manual testing and removed DO NOT MERGE Definitely don't merge this PR! labels Nov 5, 2024
workspace/apps/pidp/src/app/features/portal/portal.page.ts Outdated Show resolved Hide resolved
window.scrollTo({ top: 0 });
setTimeout(() => {
resolve();
}, 300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can define the timeout value in the constants.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function can be replaced as it exists as a service

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -68,12 +68,25 @@ export class CollegeLicenceInformationPage implements OnInit {
this.title = this.route.snapshot.data.title;
}

public scrollToTop(): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use scrollTopWithDelay from UtilsService instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Paahn
Copy link
Collaborator

Paahn commented Nov 18, 2024

The pop up doesnt open when the first time user selects None as their college licence declaration. Is this expected behaviour, do we only want to show it for users who have a college licence?

@AdityaMantripragada
Copy link
Collaborator Author

The pop up doesnt open when the first time user selects None as their college licence declaration. Is this expected behaviour, do we only want to show it for users who have a college licence?

I initially thought it was expected behavior, as the pop-up might only be relevant for users with college licenses. If you think it should appear for all users, even those selecting ‘None,’ I’ll explore updating this flow. What do you suggest?

@AdityaMantripragada AdityaMantripragada added DO NOT MERGE Definitely don't merge this PR! and removed Ready For Review PR is code-complete (or very close) and only needs some review and/or manual testing labels Nov 22, 2024
Copy link

sonarqubecloud bot commented Dec 4, 2024

@AdityaMantripragada AdityaMantripragada added Ready For Review PR is code-complete (or very close) and only needs some review and/or manual testing and removed DO NOT MERGE Definitely don't merge this PR! labels Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review PR is code-complete (or very close) and only needs some review and/or manual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants