-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
ui: remove remaining links to old db pages #136311
Conversation
In the plan details of the sql statement details page we had a remaining link to the old db page. At this time we cannot link to the new pages since the sql statement plan details api does not return the id of the db or table it belongs to. Epic: none Fixes: cockroachdb#134581 Release note (ui change): Link in plan details page to legacy table page has been removed.
{table} | ||
</Link> | ||
: {indexesList} | ||
{table}: {indexesList} |
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.
note: I recently fixed indexStatsLink that generates the wrong link for CC console, and looking at the indexesList
links, it looks like that is wrong as well (I think this is true for all cluster versions, not 24.3, so this isn't a regression due to the db pages work). I created this issue: #136476
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.
Ack - thanks for filing! I can address that in a follow-up.
@@ -356,7 +356,7 @@ export function EncodeUriName(name: string): string { | |||
return encodeURIComponent(name).replace(/%25/g, "%252525"); | |||
} | |||
|
|||
export function EncodeDatabasesUri(db: string): string { | |||
function encodeDatabasesUri(db: string): string { |
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.
Nice, i've noticed this a lot in our frontend code. Do you think we should check for this in our linter?
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.
Are you talking about the capitalization? Yeah, we could introduce a warning to lightly guide future var naming. I recall Andrii did some work to sync some linting rules between CC and db console so I'm surprised this doesn't exist already.
TFTR! |
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #134581: branch-release-24.3. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
In the plan details of the sql statement details page we had a remaining link to the old db page. At this time we cannot link to the new pages since the sql statement plan details api does not return the id of the db or table it belongs to.
Epic: none
Fixes: #134581
Release note (ui change): Link in plan details page to legacy table page has been removed.