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

Merged
merged 2 commits into from
Dec 20, 2024
Merged

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 amysorto force-pushed the url-change branch 2 times, most recently from f1ee798 to eda4edc Compare December 11, 2024 22:19
@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.

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.

LGTM! Have you tests all routes manually?

@@ -186,7 +180,7 @@ export class NavigationService {
taskId: string
) {
this.router.navigateByUrl(
`${NavigationService.SURVEY_SEGMENT}/${surveyId}/${NavigationService.LOI_SEGMENT}/${loiId}/${NavigationService.SUBMISSION_SEGMENT}/${submissionId}/${NavigationService.TASK_SEGMENT}/${taskId}`
`${SURVEY_SEGMENT}/${surveyId}/${LOI_SEGMENT}/${loiId}/${SUBMISSION_SEGMENT}/${submissionId}/${TASK_SEGMENT}/${taskId}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: Using taskId to identify the result will become problematic once we allow repeated tasks. That's out of scope for this PR, just wanted to call it out for future changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know! Currently the taskId is used just to highlight the task

@amysorto amysorto merged commit 1b6ecd5 into google:master Dec 20, 2024
2 checks passed
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