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.

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

Add formatting to cluster repos branch names #12497

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

Conversation

momesgin
Copy link
Member

@momesgin momesgin commented Nov 5, 2024

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/a for 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

image

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@momesgin momesgin added this to the v2.11.0 milestone Nov 5, 2024
@momesgin momesgin self-assigned this Nov 5, 2024

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put NA in Branch column instead of -
2 participants