cdb(front): Fetch left sidebar links #6953
Conversation
google-oss-prow
bot
requested review from
kimwnasptd,
tasos-ale and
thesuperzapper
orfeas-k
force-pushed
the
feature-orfeas-cdb-left-sidebar-links
branch
from
a1f016c
to
b84fa10
Compare
| ) {} | ||
|
|
||
| ngOnInit() { | ||
| this.envInfoSub = this.backend.getEnvInfo().subscribe((res: envInfo) => { | ||
| this.handleEnvInfo(res); | ||
| }); | ||
|
|
||
| this.dashboardLinksSub = this.backend |
There was a problem hiding this comment.
I don't think we need the dashboardLinksSub since we are not polling.
| /** | ||
| * We need this condition because Pipelines Web App removes | ||
| * the 's' from its path when navigating inside a Recurring | ||
| * Run's details page | ||
| */ | ||
| if ( | ||
| browserUrl.includes('pipeline/#/recurringrun') && | ||
| url.includes('/pipeline/#/recurringruns') | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
It is ok for now, but we may need to do something better here. I would prefer not to have hardcoded values. Maybe we can have pipeline/#/recurringrun inside menuLinks and compare it with startsWith().
There was a problem hiding this comment.
Looking at this again I don't think we should have specific code for pipelines or any other app. We could tackle this problem with another solution, but for now, let's remove this if statement.
There was a problem hiding this comment.
ACK I agree that this is not something we should tackle in the Dashboard's logic and having ifs for different WAs isn't a good practice.
cc @kimwnasptd for awareness
| return true; | ||
| } | ||
|
|
||
| return browserUrl.includes(url); |
There was a problem hiding this comment.
It works, but it will be safer to use startsWith() in case we end up with a url inside the browserUrl that we don't want to match
There was a problem hiding this comment.
Sure, I removed the prefix from browser's URL and changed that to startsWith().
| const urlParams = urlObject.search; | ||
| /** | ||
| * We need an empty string as a default value because in case | ||
| * of undefined strings, Typescript will go ahead and append the |
There was a problem hiding this comment.
Blame JavaScript not TypeScript for this 😃
| public externalLinks: any[]; | ||
| public quickLinks: quickLink[]; |
There was a problem hiding this comment.
Why we don't render externalLinks and quickLinks anywhere? If we will have them in a following PR, fine by me.
There was a problem hiding this comment.
In the old dashboard, quickLinks and documentationItems are part of the Home page and this page will be a separate effort/PR. @kimwnasptd What about externalLinks?
There was a problem hiding this comment.
@orfeas-k There is no need to wait. If we want the externalLinks we can add them in the next PR.
| export interface quickLink { | ||
| text: string; | ||
| desc: string; | ||
| link: string; | ||
| } | ||
|
|
||
| export interface documentationItem { | ||
| text: string; | ||
| desc: string; | ||
| link: string; | ||
| } |
There was a problem hiding this comment.
Maybe we can use the same type here. I don't see a need for two different interfaces.
- Fetch left sidebar links from 'api/dashboard-links' endpoint - Handle links with fragment/hash in Router navigation and URLs comparison/syncing Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
Update UI tests to mock requests at 'api/workgroup/env-info' and '/api/dashboard-links'. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
orfeas-k
force-pushed
the
feature-orfeas-cdb-left-sidebar-links
branch
from
c93bab8
to
4b9f50e
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tasos-ale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR is part of the Rewrite the Central Dashboard in Angular effort and implements the following two functionalities:
Here, we also had to handle the until now unhandled fragment/hash in links. For that, we decided to always split links to its sub-parts (
path,queryParams,fragment) and use thefragmentoption inrouter.navigate.We couldn't go only with
routerLinkActivefor this and used a custom implementation instead since we need thepath + fragmentcombination to be a subset of the browser's URL. In this implementation, we also added an explicit condition for checking when the user is in Recurring Runs WA since the KFP removes the 's' from its path when navigating inside a Recurring Run's details page (/_/pipeline/#/recurringrunsin contrast to/_/pipeline/#/recurringrun/details/).