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

Added case insensitive filtering in HTML tables #1004

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

JawHawk
Copy link
Member

@JawHawk JawHawk commented Feb 28, 2024

Fixes #924 .

Converting strings to lowercase first and then comparing for case insensitive filtering.

Also fixed the Reference error caused by 47cd777. I apologize for my oversight.

@PhilippWendler
Copy link
Member

Thanks!

Case insensitive comparisons can be somewhat tricky, because not in all languages there is a 1:1 correspondence between lowercase and uppercase characters. This is why calling toLowerCase() might not produce the desired behavior for all locales. See for example the extensive explanation at https://www.b-list.org/weblog/2018/nov/26/case/

Is there a way in JS to do a case insensitive comparison directly instead? Or at least case folding as described in the blog post?

@PhilippWendler
Copy link
Member

I am not sure, but I think that the current approach only changes the "id" filters, right? We also can have other textual columns, and we should have the same matching behavior there.

And I think it would be really good if we could get some unit tests for this. There are already some in src/tests/, also for filters, maybe they just need some extensions? If you could have a look this would be greatly appreciated.

@JawHawk
Copy link
Member Author

JawHawk commented Feb 29, 2024

Is there a way in JS to do a case insensitive comparison directly instead? Or at least case folding as described in the blog post?

There is a method toLocaleLowerCase which converts the provided string to lower case according to any locale-specific case mappings. There's also a way using normalize as suggested in blog. It may not be exact same as that of case folding, but I think most of the cases would be satisfied. Which one is preferable ?

@JawHawk
Copy link
Member Author

JawHawk commented Feb 29, 2024

I am not sure, but I think that the current approach only changes the "id" filters, right? We also can have other textual columns, and we should have the same matching behavior there.

Yes, will do the necessary changes.

And I think it would be really good if we could get some unit tests for this. There are already some in src/tests/, also for filters, maybe they just need some extensions? If you could have a look this would be greatly appreciated.

I found the filters.test.js in src/tests. but I didn't understand exactly what to do. Can you please elaborate.

@PhilippWendler
Copy link
Member

The problem with toLocaleLowerCase would be that it just works well with that locale then. normalize could improve things, but it seems it is not the same. Is there maybe a library that implements proper case-insensitive string matching? We are certainly not the only project that wants to have this requirement.

I found the filters.test.js in src/tests. but I didn't understand exactly what to do. Can you please elaborate.

Right, these are the unit tests for applyMatcher. So we can add some test cases there that call applyMatcher for the textual matching cases, right?

@JawHawk
Copy link
Member Author

JawHawk commented Feb 29, 2024

Got it, will add the textual matching test cases soon.

I only found two npm packages which do the case folding - @ar-nelson/foldcase and fold-case but they are not popular. Will they work ?

@PhilippWendler
Copy link
Member

Hm, this seems more complex than I thought. The two packages you found also only have relatively old releases.

The one alternative I found would be to use a regexp with case insensitive matching. We just have to make sure that all the special characters in the search string are escaped. And we should probably create an regexp instance and reuse it for matching across all cells instead of compiling the regexp every time. Do you think that would be doable?

@JawHawk
Copy link
Member Author

JawHawk commented Mar 1, 2024

Yes, will try to do it soon.

@JawHawk
Copy link
Member Author

JawHawk commented Mar 2, 2024

Please review the changes made.

Copy link
Member

@PhilippWendler PhilippWendler 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, just minor suggestions!

I like the test with the special character. How about also adding a test with some characters that are special for regexps, in order to test that the escaping properly works? For example, test that ... does not match anything or so.

@PhilippWendler PhilippWendler merged commit f6b8733 into sosy-lab:main Mar 5, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Case-insensitive filtering in HTML tables
3 participants