-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
- 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'; | ||
} |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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 strong thoughts here? Feels like its getting a bit too DataTabley in a way. |
There was a problem hiding this 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'; | ||
} |
There was a problem hiding this comment.
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.
I think DataTable is being replaced with |
What changed?
Refactoring the
<Explorer />
fields a bit.extraColumns
, you pass in all the columns and user can filter them as they likeid
field to columns to allow easier filtering of columnsid
field can be used to simplify ordering logic a little bitWhy was this change made?
So we can remove some fields (tenancy), in the
ClusterDiscovery
work ongoing..How was this change implemented?
How did you validate the change?
yarn tsc
+yarn lint
+yarn test
all still passtilt up
and sorting in explorer etc all still worksConcerns