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

Support removing columns from the explorer table #3611

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

foot
Copy link
Collaborator

@foot foot commented Nov 9, 2023

What changed?

Refactoring the <Explorer /> fields a bit.

  • No more extraColumns, you pass in all the columns and user can filter them as they like
  • Adds an id field to columns to allow easier filtering of columns
  • id field can be used to simplify ordering logic a little bit

Why was this change made?

So we can remove some fields (tenancy), in the ClusterDiscovery work ongoing..

How was this change implemented?

  • Refactoring and writing some tests

How did you validate the change?

  • yarn tsc + yarn lint + yarn test all still pass
  • tilt up and sorting in explorer etc all still works

Concerns

  • This maybe makes the explorer "too customisable".
    • Very open to alternative approaches to hiding columns.
      • I'm not sure about the previous solution of CSS hiding them.
  • Adding extra columns and then sorting on them seems to work but is a bit confusing to the user? Some columns are server sorted and others are client-side. Not sure best solution here.

foot added 3 commits November 9, 2023 11:41
- We now provide the default columns that can be filtered / added to etc
- Clean up sorting logic
- Don't use sortValue to derive the column id, its used to generate a
  value to sort by
// Override column name to maintain compatibility with the DataTable sync buttons
if (col === 'clusterName') {
col = 'cluster';
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about this code? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

If the sync buttons work with the new changes, this should be okay.

@foot foot added the exclude from release notes Use this label to exclude a PR from the release notes label Nov 9, 2023
@foot foot marked this pull request as ready for review November 9, 2023 11:47
@foot foot requested a review from jpellizzari November 9, 2023 17:54
@foot
Copy link
Collaborator Author

foot commented Nov 9, 2023

@jpellizzari strong thoughts here? Feels like its getting a bit too DataTabley in a way.

Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

A much more elegant solution than hiding with CSS. 👍 and 👍 👍 for adding tests!

// Override column name to maintain compatibility with the DataTable sync buttons
if (col === 'clusterName') {
col = 'cluster';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If the sync buttons work with the new changes, this should be okay.

@jpellizzari
Copy link
Contributor

@jpellizzari strong thoughts here? Feels like its getting a bit too DataTabley in a way.

I think DataTable is being replaced with TableView soon:

https://github.com/weaveworks/weave-gitops/blob/8b6addb85de22a94f00c27c32ee2160c56d6cfd2/ui/components/DataTable/TableView/TableView.tsx#L7

@foot foot merged commit 1df2976 into main Nov 13, 2023
10 checks passed
@foot foot deleted the allow-removing-explorer-columns branch November 13, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from release notes Use this label to exclude a PR from the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants