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

[ui] Search results are overloading filter with sorted results #18053

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Jul 24, 2023

Bit of context here:

This PR attempts to both preserve the ability to sort after searching, and not overload results with searched-but-not-ordered filter results by creating a new getter() that checks to see if any sorting has been applied.

@philrenaud philrenaud added the backport/1.6.x backport to 1.6.x release line label Jul 24, 2023
@philrenaud philrenaud self-assigned this Jul 24, 2023
@philrenaud philrenaud linked an issue Jul 24, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Jul 24, 2023

Ember Test Audit comparison

main da8f0de change
passes 1498 1499 +1
failures 1 1 0
flaky 0 0 0
duration 000ms 000ms -000ms

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, just missing a changelog entry and that missing property.

But I think there may be a challenge in terms of being able to get the list sorted by the original search score, since when we sort results there's no way to "unsort" them.

One approach I though was for a search to always reset the sort order to the fuzzy score, but that could be annoying? But also, that's kind of what I expect to happen? 😅

@@ -292,9 +295,24 @@ export default class IndexController extends Controller.extend(
});
}

@computed('sortProperty', 'searchTerm')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@computed('sortProperty', 'searchTerm')
@computed('sortProperty', `sortDescending`, 'searchTerm')

@philrenaud
Copy link
Contributor Author

@lgfa29

always reset the sort order to the fuzzy score

My worry here is that:

  • search fai, both "fails" and "fakepy" show up, in that order
  • click to sort by priority or something to change order
  • nothing changes because fuzzy score isn't affected

I think that would break the expected experience from #14872, so I'm apprehensive to apply it.

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 25, 2023

Ah sorry, I think I didn't explain myself well. From your example the flow I was thinking would be like this:

  1. search fai, fails, fails-2, and fakepy show up, in that order
  2. click to sort by priority or something to change order
  3. table is sorted according to selection
  4. search for fail, only fails and fails-2 show up, ordered by their fuzzy score and previous order selection is removed.

So after a search users can sort results by whichever column they would like, but changing the search term resets the ordering to fuzzy score.

@philrenaud
Copy link
Contributor Author

Got it, so what I'm currently calling unsortedSearchState might be better served as prioritizeSearchOrder or something, and is defaulted if the last thing that's taken place is a search. That sound right?

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 26, 2023

Hum...I think what's missing is a listener for changes to the search term that resets sortProperty and sortDescending to their default values. Then I believe unsortedSearchState (or prioritizeSearchOrder, it does sound like a better name) and sortedJobs would do the flow I described.

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 28, 2023

I think we still need to clear the sort query param to make the arrow disappear from the table header:
image

@philrenaud
Copy link
Contributor Author

I think we still need to clear the sort query param to make the arrow disappear from the table header:
Only thing I'm worried about there is upon removing the search term, I expect the sort to be maintained. I could hide the arrow while the search is the most recent thing, butI'd need to re-establish it once the search term is removed.

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 31, 2023

Only thing I'm worried about there is upon removing the search term, I expect the sort to be maintained.

Wouldn't that be a new search? I think I would expect the table to reset to its original state (sorted by CreateIndex) 🤔

@philrenaud
Copy link
Contributor Author

Yeah fair enough! That's actually the state I moved it to this morning, so I think I'll move forward with this.

@philrenaud philrenaud merged commit 66649d1 into main Jul 31, 2023
11 checks passed
@philrenaud philrenaud deleted the 18030-ui-search-results-are-overloading-filter-with-sorted-results branch July 31, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.6.x backport to 1.6.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ui] Search results are overloading filter with sorted results
2 participants