Add formatting to cluster repos branch names #12497
Conversation
|
|
||
| computed: { | ||
| clusterRepoBranch() { | ||
| return this.value?.isOciType ? this.t('generic.na') : this.value?.spec?.gitBranch ? this.value.spec.gitBranch : undefined; |
There was a problem hiding this comment.
We might want to pause on this.
When the list is updated to use server-side pagination the sort and search will be broken (given both will still be on value.spec.gitBranch).
The alternative is to not use server-side pagination for this list, given it's small
There was a problem hiding this comment.
Even without this change we can have missing gitBranch in the spec object. In those cases we currently show – for all types of cluster repos, but here we show n/a for just the oci type, is there a way to test the server-side pagination now?
There was a problem hiding this comment.
Server side sort and filtering can only be done on values that natively exist in the resource server side.
Here we would be showing n/a but still sorting and filtering on spec.gitBranch's undefined value. Sorting might seem ok UX wise but wouldn't strictly be true (aaa1, na1, na2, x1, n/a, n/a, n/a, n/a). The user wouldn't wouldn't get any results if they filtered on n/a.
Maybe it's just not worth converting this list. Maybe one for UX
- Support both server-side paginated lists and non-server-side pagination lists in the same session
- Support only server-side paginated lists but remove functionality like above
shell/config/product/apps.js
Outdated
| name: 'branch', | ||
| labelKey: 'tableHeaders.branch', | ||
| sort: 'spec.gitBranch', | ||
| getValue: (row) => row, |
There was a problem hiding this comment.
doesn't this break local sorting / searching?
There was a problem hiding this comment.
This breaks search (n/a provides no hits).
Would suggest this, which means the formatter might not be neeed
const clusterRepoBranch = {
name: 'branch',
labelKey: 'tableHeaders.branch',
getValue: (row) => row?.isOciType ? this.t('generic.na') : row?.spec?.gitBranch || undefined,
dashIfEmpty: true,
};
would just need to resolve access to t, which could worst case come from the model (or change to a branchLabel getter)
There was a problem hiding this comment.
store was already accessible so I used getters from it. Did you intentionally remove sort in your suggested code?
…terrepo-branch-formatter
…terrepo-branch-formatter
Summary
Fixes #12484
Occurred changes and/or fixed issues
Technical notes summary
Created a new formatter for cluster repo branch names to handle different types of repos in oder to display different text for different cases, e.g.
n/afor OCI type.Areas or cases that should be tested
Unit tests should cover all the cases but a quick manual test would also be good.
Areas which could experience regressions
Make sure the values for helm urls and git repos are displayed as before on the branch column.
Screenshot/Video
Checklist