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

Allow consumers to customize the sorting logic of the Table components #425

Closed
cjcenizal opened this issue Feb 20, 2018 · 4 comments
Closed
Assignees

Comments

@cjcenizal
Copy link
Contributor

Per discussion with @bmcconaghy, we may want to customize the way the columns of a table sort the rows within that table. For example, if that row contains JSON maybe we can sort based on a specific field within the JSON.

One possible solution would be to add a getSortValue property to the columns Table prop. The consumer can define a function which accepts the cell and row as arguments, and returns a value with which the Table can perform sort comparisons.

@uboness
Copy link
Contributor

uboness commented Feb 20, 2018

Not sure... eg. I think it doesn't make sense to let the user sort on a column named "first name" and the actual sort will be done on some other property.

The column.field property should accept a path... So together with column.render you can support the described scenario, no?

I think it makes sense to have a defaultSortField under the sorting prop (that can be a path into the field value)

For basic table, the sort needs to work both on ES and the browser. So putting custom js logic there makes little sense. With the new EuiInMemoryTable that would make more sense... But even then I'd think twice whether you can just use a field path instead.

@bmcconaghy
Copy link
Contributor

I think this was a poorly chosen example. A real example from index management is sorting a field whose value is size in bytes scaled to correct unit (10kb, 100mb, 9tb, etc). so we want to sort on actual byte value, which I calculate and then sort on that value. Another example: API returns strings a lot of times when the value is numeric. So, forcing numeric value on the string before sorting avoids all the dumb bugs with textual sort of numeric values.

@uboness
Copy link
Contributor

uboness commented Feb 21, 2018

few comments:

A real example from index management is sorting a field whose value is size in bytes scaled to correct unit (10kb, 100mb, 9tb, etc). so we want to sort on actual byte value, which I calculate and then sort on that value

do you mean that you get the value from ES already "rounded" as strings (e.g "10kb", "9tb")? This should only be the retuned when human param is passed (by default you should get the actual byte values)...

Another example: API returns strings a lot of times when the value is numeric. So, forcing numeric value on the string before sorting avoids all the dumb bugs with textual sort of numeric values.

If a value is supposed to be numeric, the API should return a number, not a string and if that's not the case, I'd suggest consulting the ES team about it to understand why it's like that (maybe there's a reason, maybe it's a bug).

In any case, in an ideal world we wouldn't need to this change for the described scenarios - but we're not leaving in an ideal world, and even if these described behaviours of ES will be fixed (if they need to be fixed) in the future, we still need to support older APIs.

One option is to translate/fix the data as it's being fetched before it's passed down to the table. Would that work?

If we want to go ahead and this support in the table, then I'd say this needs to only go in the EuiInMemoryTable that was just pushed as the EuiBasicTable should work well with server side sorting (in fact, sorting is done outside of the this table anyway) and javascript code is only good for the client. There are two ways to go about it:

Introduce a sort value callback per column - I'd incorporate this with the existing sortable such that it can either be a boolean (like it is today) or an object:

columns={[
  {
    sortable: {
      value: (item) => fetch the sort value from the item
    }
  }
]}

This means that you'll start creating "gaps" between basic table and in-memory table... some properties that should be common between the two will be supported by one and not by the other... I'm not a big fan of that (the columns prop is a good example for that).

Another option that we can consider is to introduce the sortValue function as a setting under the sorting prop (not the column prop).... something like:

<EuiInMemoryTable
  ...
  sorting={{
    sortValue: (field, item, value = undefined) => {
      switch(field) {
        case 'count':
          return Number(item.stringThatLooksLikeANumber)
        default:
          return value;
      }
    }
  }}
/>

this one is a big more "verbose" but doesn't clutter the columns configuration but centralized this functionality in one place for all the columns (now you have column specific logic in 2 places).

Not a big fan of either way... so if we could just prep the data better before we pass it to the table, that'd be great... if we really can't, I think I'd prefer the first option (enabling defining sortable as an object on each column), just because that's the natural place of such callback function (btw... for the same reason, the sortable field was added to the column in the first place... I could have just listed all the sortable columns under the sorting prop... but that feels unnatural and off.

@peteharverson
Copy link

👍 I am currently converting the table of anomalies in the ML UI to EUI / React and there are three columns where I could do with this functionality.

  • actual and typical fields of the anomaly - the underlying data is an array, which normally contains a single value. However for some anomaly detector functions, such as lat_long this is a multi-valued array. The current sort behavior, on the array values, results in an alphanumeric sort:

image

  • description column - this column is derived from the actual and typical fields from the anomaly, giving a textual description of how the actual value compares to the typical value e.g. '3x higher', '4x lower. Being able to specify custom sort behavior would allow sorting on this column by the magnitude of the factor of how the actual compares to the typical value, as in the current table:

image

Of the two options described above, my preference would be for the first option, allowing a sortable for each column.

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

No branches or pull requests

5 participants