Central Dashboard - Add statefulness to generated links #2913
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
area/front-end
area/centraldashboard
|
addresses #2845 |
| */ | ||
| _buildHref(href) { | ||
| const current = new URL(window.location.href); | ||
| const url = new URL(href, window.location.origin); |
There was a problem hiding this comment.
Why do we need this, isn't that implicit?
| const url = new URL(href, window.location.origin); | |
| const url = new URL(href); |
There was a problem hiding this comment.
According to MDN, If url is a relative URL, base is required, and will be used as the base URL. If url is an absolute URL, a given base will be ignored. If we were getting the href from the anchor element's href property it would already be absolute, but since we are passing in a relative string we need to provide the base. The suggested change failed when I tried it.
There was a problem hiding this comment.
Ok, perhaps I misunderstood what this was doing. Thanks for following through!
| const url = new URL(e.currentTarget.href); | ||
| window.history.pushState({}, null, `_${url.pathname}`); | ||
| // e.currentTarget is an anchor element | ||
| const url = e.currentTarget.href.substr(e.currentTarget.origin.length); |
There was a problem hiding this comment.
I think slice is the recommended js-onic way of substringing:
| const url = e.currentTarget.href.substr(e.currentTarget.origin.length); | |
| const url = e.currentTarget.href.slice(e.currentTarget.origin.length); |
| paper-tab(page='activity', link) | ||
| a.link(tabindex='-1', href='/activity') Activity | ||
| aside#NamespaceSelector(hidden$='[[!equals(page, "activity")]]') | ||
| a.link(tabindex='-1', href$='[[_buildHref("/activity")]]') Activity |
There was a problem hiding this comment.
Won't these be static links?? All the _buildHref?
There was a problem hiding this comment.
I guess that depends. I was thinking that namespace would behave similar to how projectId behaves in the GCP console. It's propagated along with every link even if the page being displayed doesn't use it to change its behavior.
prodonjs
changed the title
|
/remove do-not-merge/work-in-progress |
|
@avdaredevil PTAL when you have the chance. I think this addresses things correctly. If we find that other pages are often introducing links that need to be namespace aware, we should consider moving this to a mixin along with some of the other utility methods you created. |
|
I agree I can make a PR next week to refactor a few of these changes. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avdaredevil 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 |
* Add statefulness to generated links * Ensure that _buildHref uses a computed dependency when queryParams changes * Final implementation including tests * Add logic to hide namespace selector only when viewing Pipelines
* Add statefulness to generated links * Ensure that _buildHref uses a computed dependency when queryParams changes * Final implementation including tests * Add logic to hide namespace selector only when viewing Pipelines
Ensures that all links on the Central Dashboard are namespace-aware in order to preserve state and allow it to be passed down to child components.
/area front-end
/area centraldashboard
/assign @avdaredevil
This change is