tech-story: [M3-10019] - Reroute CloudPulse - #12262
Conversation
6efe1a9 to
7d4a4d4
Compare
| ascending: [1, 2, 3, 4], | ||
| column: 'service_type', | ||
| descending: [4, 3, 2, 1], | ||
| descending: [3, 4, 1, 2], |
There was a problem hiding this comment.
The previous assertion was actually wrong. Since we only have two service types it's hard to be accurate anyway, and the order here also depends on what was just right before. Not the most reliable assertion overall.
| Typography, | ||
| } from '@linode/ui'; | ||
| import React from 'react'; | ||
| // eslint-disable-next-line no-restricted-imports |
There was a problem hiding this comment.
This is also use within the /linodes namespace. Will be addressed along with other residual react-router-dom history instances
coliu-akamai
left a comment
coliu-akamai
left a comment
There was a problem hiding this comment.
awesome thanks @abailly-akamai!
✅ when feature flag is off, NotFound is shown for metrics/alerts related routes
✅ /metrics
✅ /alerts landing + order by
- ✅ alert detail
- ✅ edit alert
- ✅ create alert
✅ code review
| <Paginate data={sortedData ?? []}> | ||
| {({ | ||
| count, | ||
| data: paginatedAndOrderedAlerts, | ||
| handlePageChange, | ||
| handlePageSizeChange, | ||
| page, | ||
| pageSize, | ||
| }) => { |
There was a problem hiding this comment.
optional: do we want to update pagination to usePaginationV2 to use routing too?
There was a problem hiding this comment.
Good question! Not really because <Paginate /> is client-side only and does not contain any react-router-dom utils. It can stay for those use cases 👍
hkhalil-akamai
left a comment
hkhalil-akamai
left a comment
There was a problem hiding this comment.
Did not observe any loss of functionality.
hkhalil-akamai
added
Approved
|
@abailly-akamai , Quick question, In our prod/alpha lets say, if we put the below URL in the browser, a wrong URL to details page It redirects to https://cloud.dev.linode.com/alerts/definitions and in the browser correct URL is shown as (https://cloud.dev.linode.com/alerts/definitions) but in my localhost in this branch if I ping http://localhost:3000/alerts/definitions/detailss/dbaas/36 It again redirects to list page, but the URL in the browser remains http://localhost:3000/alerts/definitions/detailss/dbaas/36, which is quiet confusing, I would try to replicate the production behaviour in this case as well since it is nice without wrong URL's Like this we need to take care of the metrics endpoints as well. |
venkymano-akamai
left a comment
venkymano-akamai
left a comment
There was a problem hiding this comment.
@abailly-akamai , I have left a comment here, please look
|
@venkymano-akamai thx I added handling for this. Quite an edge case but at least there's parity there.
Not sure I follow about metrics, i don't think it applies there |
venkymano-akamai
left a comment
venkymano-akamai
left a comment
There was a problem hiding this comment.
Approving with a comment,
@abailly-akamai , I checked and the alerts part works fine. Since we have touched /metrics also in CloudPulseDashboardLanding.tsx, While pinging like this prod
https://cloud.linode.com/metrics/sss
It redirects to https://cloud.linode.com/metrics and lands in dashboards page.
Where is in our PR branch pinging http://localhost:3000/metrics/sss, show Not Found page. Lets make this consistent as well.
Rest of the PR looks good for me.
|
@venkymano-akamai getting a not found page is in general a better behavior than a redirect, so will keep this as is. The case with alerts was a bit different since it was dealing with params, but in the case of metrics it is a pure non-existent route. |
Cloud Manager UI test results🎉 625 passing tests on test run #14 ↗︎
|
5 participants
Description 📝
Rerouting the
/metricsand/alertsname spaces, andCloudPulseas a whole.This one is a little more straight forward due to the lack of modals/dialogs to route. There are only a couple, which could (or not) be addressed later.
Changes 🔄
Preview 📷
There should be no visual or functional regression as a result of this PR
How to test 🧪
Verification steps
Confirm no regression (UI/routing) with
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅