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

Update URL paths and remove unused code #2118

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amysorto
Copy link
Contributor

@amysorto amysorto commented Dec 5, 2024

Fixes #1243

Also removed LOI_JOB_ID_FRAGMENT_PARAM since it not currently being used

@amysorto amysorto added the web Angular implementation of Web UI label Dec 5, 2024
@amysorto amysorto requested a review from gino-m December 5, 2024 14:42
private static readonly JOB_ID_FRAGMENT_PARAM = 'job';
private static readonly LOI_ID_FRAGMENT_PARAM = 'site';
private static readonly SUBMISSION_ID_FRAGMENT_PARAM = 'submission';
private static readonly TASK_ID_FRAGMENT_PARAM = 'task';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @amysorto ! This will rename the fragments, but we also need the router to use the URI rather than URL fragment. The exact patterns are described in the issue descriptions. Please lmk if you have any questions!

@gino-m
Copy link
Collaborator

gino-m commented Dec 10, 2024

@amysorto Gentle ping!

@amysorto
Copy link
Contributor Author

I deleted everything inside pages/main-page-container/main-page/side-panel folder as well since it wasn't being used anymore and used some of the code I deleted in the navigation service.

@amysorto amysorto requested a review from gino-m December 11, 2024 22:24
Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

Some quick feedback, will do another pass tomorrow.

@@ -95,6 +95,21 @@ const routes: Routes = [
component: TermsComponent,
canActivate: [AuthGuard],
},
{
path: `${NavigationService.SURVEY_SEGMENT}/:${NavigationService.SURVEY_ID}/${NavigationService.LOI_SEGMENT}/:${NavigationService.LOI_ID}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use destructuring assignment to make these constants more readable? ie somewhere above const {SURVEY_SEGMENT, LOI_SEGMENT, ...} = NavigationService;?

this.setFragmentParams(new HttpParams({fromString: ''}));
showSubmissionDetail(surveyId: string, loiId: string, submissionId: string) {
this.router.navigateByUrl(
`${NavigationService.SURVEY_SEGMENT}/${surveyId}/${NavigationService.LOI_SEGMENT}/${loiId}/${NavigationService.SUBMISSION_SEGMENT}/${submissionId}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
web Angular implementation of Web UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve URL paths
2 participants