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 sorting on multiple columns in Discover #41918

Merged
merged 15 commits into from
Aug 2, 2019
Merged

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jul 24, 2019

Summary

Fixes #696

I took a crack at this previously and got pretty far, the only thing that blocked me was #16324. Lukas took over that PR and finished it in #33453. So I dug up my old work and merged in the latest from master. After some quick testing everything seems to be working so I'm submitting this PR.

This commit enables sorting on multiple columns in Discover and in saved search panels on dashboards. The UI is simple and should be familiar based on the way multi-sort works in other common applications like file explorers. Each sortable column has a sort icon indicating which way it is sorted (or unsorted, in the case of two arrows pointing both up and down). Sort priority is determined by which column was clicked most recently, with the most recent being the lowest priority.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@Bargs Bargs added release_note:enhancement Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.4.0 labels Jul 24, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@Bargs
Copy link
Contributor Author

Bargs commented Jul 24, 2019

@AlonaNadler @timroes I've submitted a PR with the old work I mentioned during our meeting today.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Code looks good.

I tested this in Chrome and Safari and it works as expected.

In addition, I tested an imported saved search and the sort order was maintained.
Screen Shot 2019-07-24 at 5 30 37 PM

A few small things:

  1. I keep expecting the sort order to be maintained in the order in which I added the sort (first one most important and the orders being inner/nested sorting fields). We might want to consider adding some visual indicator of the sorting precedence later on.
  2. The icons are really small and it would be nice to make them more obvious at a later stage but that requires more real estate in table headers.

# Conflicts:
#	src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/rows_headers.js
#	src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header.js
@Bargs
Copy link
Contributor Author

Bargs commented Jul 25, 2019

Hold off on review, need to resolve conflicts caused by #41259

@Bargs
Copy link
Contributor Author

Bargs commented Jul 25, 2019

Alright, conflicts are resolved and this is ready for review again. @TinaHeiligers would you mind rechecking? I basically had to reimplement the multi-sort UI in the new react table header so I think it's worth another look.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@TinaHeiligers TinaHeiligers self-requested a review July 25, 2019 19:33
@AlonaNadler
Copy link

@kertal @timroes how does this effort will work with our refactoring plans and the new table component?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs requested a review from kertal July 26, 2019 17:25
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Code LGTM.
Tested in Chrome, Safari and Firefox. All still works as expected!

const btnSortClassName =
sortDirection !== '' ? btnSortIcon : `kbnDocTableHeader__sortChange ${btnSortIcon}`;

const handleChangeSortOrder = () => {
// Cycle goes Unsorted -> Asc -> Desc -> Unsorted
Copy link
Contributor

Choose a reason for hiding this comment

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

The sort order cycle is not obvious in the UI. Perhaps we should consider adding a tooltip or some other indicator of what the cycle is.

@Bargs
Copy link
Contributor Author

Bargs commented Jul 31, 2019

I don't think we should invest too much time into gold plating the UI since the doc table will be rewritten soon anyway. I think the pattern I've implemented is pretty standard in most software. Click once, sort in one direction, click a second time, sort in the other direction, click again and it removes the sort.

This popular table component does the same thing. The only difference is that it requires the user to hold shift to do multi-sort. I think that's worse because it introduces a magic key combo and it also conflicts with a native browser behavior (selecting a block of text).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally in Chrome, I think also @elastic/kibana-design should take a look at it because of UI & usability and the transformation to the new grid component. I think we should align this intermediate step

@kertal
Copy link
Member

kertal commented Aug 1, 2019

Here's a quick suggestion how we could display the sorted fields in the current layout, also it would allow an easier removal of sorted fields:

Bildschirmfoto 2019-08-01 um 14 33 45

It would add information about the order of the sorted fields, also you would see, if you click 5 different fields without removing one, that you're build up a kind of sorted fields stack (maybe we should also limit the number of fields you can sort by)

@ryankeairns
Copy link
Contributor

ryankeairns commented Aug 1, 2019

I'll start this up and take a quick look. I get where @Bargs is coming from and, as long as it fits common UI interaction patterns, then it seems a valuable (small, yet impactful) change that need not get bogged down by too much UI work at this stage.

@ryankeairns
Copy link
Contributor

Here are a few (hopefully small) things I would recommend changing:

  1. Make the currently sorted columns more apparent: change the color of the icon (when activated) from the light gray to another EUI color (primary blue, secondary green, or the accent pink).

Screenshot 2019-08-01 11 39 21

  1. Make the tooltip text more informative: I see there is a 'dynamic' aria-label on the button element that would be great to use in the tool tip as it provides specific wording on the next sort action (e.g. Sort taxful_total_price ascending):

Screenshot 2019-08-01 11 40 25

  1. Consider flipping the sort priority.
    For me, I feel like the first sort I do should be the highest priority, then each subsequent sort is a sort within that set. For example, if I sort by make of car, then by number of doors, I'd like that to be a sort by/within the make. While working with sample data, I found my self doing a second sort, then having to go back and re-do my initial sort.

For future reference, we may be heading towards a more robust search that looks something like these examples, both of which also keep the initial sort the top priority:

Airtable sorting

Screenshot 2019-08-01 11 27 10

Google Sheets sorting

Screenshot 2019-08-01 11 29 51

@kertal
Copy link
Member

kertal commented Aug 2, 2019

@ryankeairns "For future reference, we may be heading towards a more robust search that looks something like these examples, both of which also keep the initial sort the top priority"

it might be a weird kind of i18n or that my morning caffein doesn't work, but I've got the impression, that in case of Airtable, always the latest click gets highest priority?

@Bargs Bargs requested a review from a team as a code owner August 2, 2019 15:09
@Bargs
Copy link
Contributor Author

Bargs commented Aug 2, 2019

Thanks for the suggestions @ryankeairns, I think they're all good ones and I've implemented them all.

  • Sort buttons now have an active color
  • Tool tips are more descriptive
  • Most recently clicked sort button now has lowest priority

image

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit f6cb00a into elastic:master Aug 2, 2019
Bargs added a commit to Bargs/kibana that referenced this pull request Aug 2, 2019
This commit enables sorting on multiple columns in Discover and in saved search panels on dashboards. The UI is simple and should be familiar based on the way multi-sort works in other common applications like file explorers. Each sortable column has a sort icon indicating which way it is sorted (or unsorted, in the case of two arrows pointing both up and down). Sort priority is determined by which column was clicked most recently, with the most recent being the lowest priority.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 5, 2019
…-or-edit-existing-rollup-job

* 'master' of github.com:elastic/kibana: (67 commits)
  [TSVB] Shim new platform (elastic#39169)
  [Metric Vis] Shim new platform (elastic#42240)
  [Tag Cloud] Shim new platform (elastic#42348)
  Disable flaky request lib tests. Add es_ui_shared plugin to CODEOWNERS.
  Add disk space percentage to node listing (elastic#42145)
  [SIEM] Add chart interactions - update date picker after brush selection on charts (elastic#42440)
  Document HTTP service (elastic#42331)
  [Reporting] Sanitize 409 error log message (elastic#42495)
  [docs][skip ci] Maps read only access (elastic#35561)
  [x-pack/ftr] refactor types to be more accurate/consistent wit… (elastic#42407)
  [DOCS] Updates images and content in Dashboard docs (elastic#42500)
  Allow sorting on multiple columns in Discover (elastic#41918)
  [Infra UI][Logs UI] Fix autocomplete to use proper derived index pattern (elastic#42287)
  [ftr/cheerio] improve cheerio types to include test subject me… (elastic#42534)
  Upgraded EUI 13.0.0 -> 13.1.1 (elastic#42298)
  Increase max-old-space-size for builds (elastic#42218)
  [Infra UI] Add cloud metrics and cloud/host info to metadata endpoint (elastic#41836)
  [Logs UI][a11y] Announce name of column on remove column button (elastic#41695)
  Inspector 👉 New Platform (elastic#42164)
  Make alerting properly space aware (elastic#42081)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 5, 2019
…s_autocomplete

* 'master' of github.com:elastic/kibana: (189 commits)
  [TSVB] Shim new platform (elastic#39169)
  [Metric Vis] Shim new platform (elastic#42240)
  [Tag Cloud] Shim new platform (elastic#42348)
  Disable flaky request lib tests. Add es_ui_shared plugin to CODEOWNERS.
  Add disk space percentage to node listing (elastic#42145)
  [SIEM] Add chart interactions - update date picker after brush selection on charts (elastic#42440)
  Document HTTP service (elastic#42331)
  [Reporting] Sanitize 409 error log message (elastic#42495)
  [docs][skip ci] Maps read only access (elastic#35561)
  [x-pack/ftr] refactor types to be more accurate/consistent wit… (elastic#42407)
  [DOCS] Updates images and content in Dashboard docs (elastic#42500)
  Allow sorting on multiple columns in Discover (elastic#41918)
  [Infra UI][Logs UI] Fix autocomplete to use proper derived index pattern (elastic#42287)
  [ftr/cheerio] improve cheerio types to include test subject me… (elastic#42534)
  Upgraded EUI 13.0.0 -> 13.1.1 (elastic#42298)
  Increase max-old-space-size for builds (elastic#42218)
  [Infra UI] Add cloud metrics and cloud/host info to metadata endpoint (elastic#41836)
  [Logs UI][a11y] Announce name of column on remove column button (elastic#41695)
  Inspector 👉 New Platform (elastic#42164)
  Make alerting properly space aware (elastic#42081)
  ...
Bargs added a commit that referenced this pull request Aug 5, 2019
This commit enables sorting on multiple columns in Discover and in saved search panels on dashboards. The UI is simple and should be familiar based on the way multi-sort works in other common applications like file explorers. Each sortable column has a sort icon indicating which way it is sorted (or unsorted, in the case of two arrows pointing both up and down). Sort priority is determined by which column was clicked most recently, with the most recent being the lowest priority.
@darshakkakkad
Copy link

Hello, from which release of Kibana is this feature available?

@nickofthyme
Copy link
Contributor

@darshakkakkad released with 7.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow sorting on multiple fields
8 participants