Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add formatting to cluster repos branch names #12497

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add formatting to cluster repos branch names #12497

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dce0e8e
add formatting to cluster repos branch names
Nov 4, 2024
b887371
Merge branch 'master' of github.com:rancher/dashboard into 12484-clus…
Nov 13, 2024
a7d09a2
replace formatter with a condition inside getValue
Nov 15, 2024
8bb3315
Merge branch 'master' of github.com:rancher/dashboard into 12484-clus…
Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions shell/components/formatter/ClusterRepoBranch.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<script>
export default {
props: {
value: { // row
type: Object,
default: () => {}
},
col: {
type: Object,
default: () => {}
}
},

computed: {
clusterRepoBranch() {
return this.value?.isOciType ? this.t('generic.na') : this.value?.spec?.gitBranch ? this.value.spec.gitBranch : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Support both server-side paginated lists and non-server-side pagination lists in the same session
  2. Support only server-side paginated lists but remove functionality like above

}
}
};
</script>

<template>
<span>
{{ clusterRepoBranch }}
<template v-if="!clusterRepoBranch && col.dashIfEmpty">
<span class="text-muted">&mdash;</span>
</template>
</span>
</template>
24 changes: 24 additions & 0 deletions shell/components/formatter/__tests__/ClusterRepoBranch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { mount } from '@vue/test-utils';
import ClusterRepoBranch from '@shell/components/formatter/ClusterRepoBranch.vue';

const notApplicableTranslationKey = '%generic.na%';

describe('component: ClusterRepoBranch', () => {
it.each([
[{ isOciType: false, spec: { url: 'https://git.rancher.io/charts', gitBranch: 'dev-v2.10' } }, 'dev-v2.10'],
[{ isOciType: false, spec: { url: 'https://git.rancher.io/charts', gitBranch: 'main' } }, 'main'],
[{ isOciType: false, spec: { url: '', gitBranch: 'main' } }, 'main'],
[{ isOciType: false, spec: { url: '', gitBranch: '' } }, '—'],
[{ isOciType: false, spec: { url: '' } }, '—'],
[{ isOciType: true, spec: { url: 'oci://' } }, notApplicableTranslationKey],
[{ isOciType: true, spec: { url: 'oci://', gitBranch: 'main' } }, notApplicableTranslationKey],
[{ isOciType: true, spec: { url: '' } }, notApplicableTranslationKey],
[{ }, '—'],
[undefined, '—'],
])('should display correct branch', async(clusterRepo, expectedBranchTextToShow) => {
const wrapper = await mount(ClusterRepoBranch, { props: { value: clusterRepo, col: { dashIfEmpty: true } } });
const element = wrapper.find('span');

expect(element.text()).toBe(expectedBranchTextToShow);
});
});
11 changes: 10 additions & 1 deletion shell/config/product/apps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,18 @@ export function init(store) {
dashIfEmpty: true,
};

const clusterRepoBranch = {
name: 'branch',
labelKey: 'tableHeaders.branch',
sort: 'spec.gitBranch',
getValue: (row) => row,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this break local sorting / searching?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't

search.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store was already accessible so I used getters from it. Did you intentionally remove sort in your suggested code?

formatter: 'ClusterRepoBranch',
dashIfEmpty: true,
};

headers(CATALOG.APP, [STATE, NAME_COL, NAMESPACE, CHART, CHART_UPGRADE, APP_SUMMARY, AGE]);
headers(CATALOG.REPO, [STATE, NAME_COL, NAMESPACE, repoType, repoUrl, repoBranch, AGE]);
headers(CATALOG.CLUSTER_REPO, [STATE, NAME_COL, repoType, repoUrl, repoBranch, AGE]);
headers(CATALOG.CLUSTER_REPO, [STATE, NAME_COL, repoType, repoUrl, clusterRepoBranch, AGE]);
headers(CATALOG.OPERATION, [
STATE,
NAME_COL,
Expand Down
Loading