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

TablePanel: Make sorting case-insensitive #32435

Merged
merged 3 commits into from Apr 7, 2021
Merged

TablePanel: Make sorting case-insensitive #32435

merged 3 commits into from Apr 7, 2021

Conversation

kaydelaney
Copy link
Contributor

What this PR does / why we need it:
Adds a sort type 'alphanumeric-insensitive' to the Table component and makes it the default for non-numeric fields.

Which issue(s) this PR fixes:
Closes #30476

Special notes for your reviewer:

@kaydelaney kaydelaney requested a review from a team March 29, 2021 14:12
@kaydelaney kaydelaney self-assigned this Mar 29, 2021
@kaydelaney kaydelaney added this to In Review in Frontend Platform Backlog via automation Mar 29, 2021
@kaydelaney kaydelaney requested review from oscarkilhed and hugohaggmark and removed request for a team March 29, 2021 14:12
Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

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

LGTM and works as expected!

@hugohaggmark
Copy link
Contributor

Friendly reminder to change title to be change log compatible, add appropriate labels, milestone and backport label so this issue gets shipped in the next patch release.

@leeoniya
Copy link
Contributor

might also consider https://stackoverflow.com/a/51005583

@kaydelaney
Copy link
Contributor Author

@leeoniya Oh that's very nice, had no idea that existed. Will change to make use of that.

@kaydelaney kaydelaney requested a review from a team March 30, 2021 11:05
@kaydelaney kaydelaney added this to the 7.5.3 milestone Mar 30, 2021
@kaydelaney kaydelaney changed the title UI/Table: Make sorting case-insensitive UI/Table: Makes sorting case-insensitive Mar 30, 2021
@kaydelaney kaydelaney added add to changelog old backport v7.5.x Mark PR for automatic backport to v7.5.x labels Mar 30, 2021
@kaydelaney kaydelaney changed the title UI/Table: Makes sorting case-insensitive TablePanel: Makes sorting case-insensitive Mar 30, 2021
@leeoniya
Copy link
Contributor

leeoniya commented Mar 30, 2021

sorry, last drive-by comment: caching the locale object outside the sort loop should be faster, too:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Collator

something like

let myCompare = new Intl.Collator('sv', { sensitivity: 'base' }).compare;

stringsArray.sort(myCompare);

@kaydelaney
Copy link
Contributor Author

sorry, last drive-by comment: caching the locale object outside the sort loop should be faster, too:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Collator

something like

let myCompare = new Intl.Collator('sv', { sensitivity: 'base' }).compare;

stringsArray.sort(myCompare);

I saw mention of that Collator thing on MDN, but I'm not sure it'll work with how react-table is set up, since you only provide a comparison function rather than sorting the arrays yourself.

@dhilt
Copy link
Contributor

dhilt commented Mar 31, 2021

Hi! We have a fork of Grafana 7.3.2 and I'm fixing the same issue. The fix you are discussing here looks good except typings. The following method

export function sortCaseInsensitive(a: Row<any>, b: Row<any>, id: string) {
  return a.values[id].localeCompare(b.values[id], undefined, { sensitivity: 'base' });
}

allows to pass non-string values which don't have "localeCompare" method. For example, null value, it will cause "SyntaxError: Invalid or unexpected token". This is the case that I faced in our environment.

@ifrost ifrost modified the milestones: 7.5.3, 7.5.4 Apr 7, 2021
@kaydelaney
Copy link
Contributor Author

Hi! We have a fork of Grafana 7.3.2 and I'm fixing the same issue. The fix you are discussing here looks good except typings. The following method

export function sortCaseInsensitive(a: Row<any>, b: Row<any>, id: string) {
  return a.values[id].localeCompare(b.values[id], undefined, { sensitivity: 'base' });
}

allows to pass non-string values which don't have "localeCompare" method. For example, null value, it will cause "SyntaxError: Invalid or unexpected token". This is the case that I faced in our environment.

Thanks for catching this! Pushed a fix

@kaydelaney kaydelaney merged commit 6c16ecf into master Apr 7, 2021
Frontend Platform Backlog automation moved this from In Review to Done Apr 7, 2021
@kaydelaney kaydelaney deleted the issue-30476 branch April 7, 2021 13:28
grafanabot pushed a commit that referenced this pull request Apr 7, 2021
* UI/Table: Make sorting case-insensitive
Closes #30476

(cherry picked from commit 6c16ecf)
dprokop pushed a commit that referenced this pull request Apr 7, 2021
* UI/Table: Make sorting case-insensitive
Closes #30476

(cherry picked from commit 6c16ecf)

Co-authored-by: kay delaney <45561153+kaydelaney@users.noreply.github.com>
xlson added a commit to joanlopez/grafana that referenced this pull request Apr 13, 2021
* origin/master: (96 commits)
  Introduce "scuemata" system for CUE-based specification of Grafana objects (grafana#32527)
  Toolkit: catch errors in version output (grafana#32774)
  safer, more idiomatic proxy helper (grafana#32732)
  Themes: V8 Theme model (grafana#32342)
  CloudWatch: replace full query parser with regex scanner that extracts stats groups (grafana#32610)
  Input: updates story from knobs to control (grafana#32712)
  Explore: correctly cleanup state on unmount (grafana#32557)
  Add timeout option to datasource config (grafana#31871)
  Chore: move errors to enterprise (grafana#32753)
  Update latest.json to 7.5.3 (grafana#32755)
  SSE: fix casing on classic conditions model. (grafana#32754)
  Improve error handling for Graphite 0.9 and 1.0 (grafana#32642)
  Table: Fixes table data links so they refer to correct row after sorting (grafana#32571)
  ReleaseNotes: Updated changelog and release notes for 7.5.3 (grafana#32749)
  TablePanel: Makes sorting case-insensitive (grafana#32435)
  Bump github.com/influxdata/influxdb-client-go/v2 from 2.2.2 to 2.2.3 (grafana#32683)
  add memcached integration test steps in starlark (grafana#32729)
  AlertingNG: Temp endpoint to translate dashboard alert into rule group (grafana#32694)
  Prometheus: Show only graph for range queries in Explore (grafana#32489)
  LibraryPanels: Add search input back to add panel widget (grafana#32719)
  ...
@oddlittlebird oddlittlebird changed the title TablePanel: Makes sorting case-insensitive TablePanel: Make sorting case-insensitive Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sorting table values case-insensitive
6 participants