-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Tech Debt] Use consts for routes #160521
[Tech Debt] Use consts for routes #160521
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
fa96b3d
to
65066e2
Compare
65066e2
to
ec563a8
Compare
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM!
Added one question
x-pack/plugins/observability/public/pages/alert_details/alert_details.tsx
Show resolved
Hide resolved
features={{ alerts: { sync: false, isExperimental: false } }} | ||
owner={[observabilityFeatureId]} | ||
permissions={permissions} | ||
ruleDetailsNavigation={{ | ||
href: (ruleId) => prepend(paths.observability.ruleDetails(ruleId)), | ||
href: (ruleId) => prepend(paths.observability.ruleDetails(ruleId || '')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What will happen in case of not having a ruleId? Will we show an error on the page?
I wonder why we didn't need to have this before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, in /config/paths.ts
, we had:
...
ruleDetails: (ruleId?: string | null) =>
ruleId ? `${RULES_PAGE_LINK}/${encodeURI(ruleId)}` : RULES_PAGE_LINK,
...
This has now made more strict as well as consistent with the other details pages (Alert, SLO):
ruleDetails: (ruleId: string) => `${OBSERVABILITY_BASE_PATH}${RULES_PATH}/${encodeURI(ruleId)}`,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will show in this case for rule details if there is no ruleId
?
Resolves #158146
Summary
This consolidates the route configuration of the Observability app.
Changes:
/config/paths.ts
to/routes/paths.ts
so they are closer together;/routes/index.tsx
to/routes/routes.tsx
;