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

Fixed case-sensitive sorting for KTable #854

Merged
merged 7 commits into from
Dec 16, 2024

Conversation

BabyElias
Copy link
Contributor

@BabyElias BabyElias commented Dec 7, 2024

Description

This PR fixes the issue caused by case-sensitive sorting for KTable where uppercase characters were sorted before lowercase characters. toLocaleLowerCase() method has been used to ensure internationalisation concerns are addressed while standardising strings to lower case for comparison.

Issue addressed

Addresses #852

Before/after screenshots

Before -
table sort
After -
table sort 2

Changelog

Does this introduce any tech-debt items?

No

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

@MisRob
Copy link
Member

MisRob commented Dec 9, 2024

Thank you for keeping taking care of the table, @BabyElias :) I will invite my colleague @AllanOXDi for review. One suggestion from me would be to add a unit test to cover this, if you'd like to do so meanwhile.

@MisRob MisRob requested a review from AllanOXDi December 9, 2024 09:02
@MisRob
Copy link
Member

MisRob commented Dec 9, 2024

@AllanOXDi for reference, this issue #852 was open by @BabyElias after we agreed together it's expected behavior.

@MisRob MisRob added the TODO: needs review Waiting for review label Dec 9, 2024
@BabyElias
Copy link
Contributor Author

I have updated the test cases to cover strings having both lower and uppercase characters.

@MisRob MisRob requested a review from AllanOXDi December 16, 2024 07:08
* @param {Number} index - The index of the column to sort by.
* @returns {any} - The value to be used for sorting.
*/
const getSortValue = (row, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

Neat

@MisRob
Copy link
Member

MisRob commented Dec 16, 2024

This looks good to me too. @AllanOXDi feel free to merge after you've seen the latest changes if you think all is ready :) Thanks both!

@MisRob
Copy link
Member

MisRob commented Dec 16, 2024

I made a small tweak to the changelog "Description" to be a bit more specific.

Copy link
Member

@AllanOXDi AllanOXDi left a comment

Choose a reason for hiding this comment

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

Looks good to me too! Thanks @BabyElias

@AllanOXDi AllanOXDi merged commit 901a5a1 into learningequality:develop Dec 16, 2024
12 of 15 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants