-
Notifications
You must be signed in to change notification settings - Fork 842
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
Added isSortable to Datagrid columns #2952
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Thanks for this! Two things:
Currently, most of these settings come from the schema detector definition, but that can be cumbersome to work with and we are going to move to columns overriding individual settings like |
@chandlerprall I have updated the changes you have suggested. Can you please review the changes. |
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.
Changes look good, just one thing to clean up
{inactiveColumns.length > 0 && ( |
needs to be updated to consider the length of sortable columns, otherwise the Pick fields to sort by UI can end up as an empty drop down instead of removed from the window.
To help consolidate the new logic, it's probably best to create a new array (e.g. inactiveSortableColumns
or similar) based on the isSortable
props, and use that array when rendering choices and showing/hiding selection popover.
@chandlerprall Changes updated. Can you please review? |
jenkins test this |
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.
Schema detector test at
eui/src/components/datagrid/data_grid.test.tsx
Lines 907 to 920 in 9bc89c0
schemaDetectors={[ | |
{ | |
type: 'ipaddress', | |
detector(value: string) { | |
return value.match(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/) | |
? 1 | |
: 0; | |
}, | |
icon: 'alert', | |
color: 'primary', | |
sortTextAsc: 'a-z', | |
sortTextDesc: 'z-a', | |
}, | |
]} |
isSortable: true
Sorry for the back-and-forth - should be the last 2 items, then I'll merge this in. |
@chandlerprall No problem. Can you please review it? |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2952/ |
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.
LGTM! Pulled & tested locally.
Summary
Fixes #2950
Checklist