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

enable case insensitive on segment search #18643

Merged
merged 24 commits into from Jan 20, 2022
Merged

Conversation

peterhashair
Copy link
Contributor

Description:

Fixes: #18640
enable case insensitive on segment search

Review

@peterhashair peterhashair added this to the 4.7.0 milestone Jan 18, 2022
@peterhashair peterhashair added the Needs Review PRs that need a code review label Jan 18, 2022
@peterhashair
Copy link
Contributor Author

peterhashair commented Jan 18, 2022

@diosmosis did a simple update for segment insensitive search. Hopefully, I did it right.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jan 18, 2022
@sgiehl
Copy link
Member

sgiehl commented Jan 18, 2022

btw @peterhashair As the issue is meant to be included in the 4.7.0 final, your pull request should have been based on and targeted to the next_release branch. I will rebase your branch and change the PR base. You may need to do a hard reset to the origin before doing any further changes though

@sgiehl sgiehl changed the base branch from 4.x-dev to next_release January 18, 2022 09:23
Peter added 2 commits January 18, 2022 10:25
enable case insentive
shorter the function
@sgiehl
Copy link
Member

sgiehl commented Jan 18, 2022

@peterhashair This looks good for me now.
For regressions its always good to check if we can adjust our tests so this will be covered for the future. Not sure if we already have a UI test that uses such a field for searching, but guess that could be added to the SegmentSelectorEditor tests maybe.

add test for auto complete
@justinvelluppillai
Copy link
Contributor

Maybe would be better to have a UI test that performs a search with a capital letter in it and then checks the results are the same?

Peter added 3 commits January 20, 2022 03:02
update tests
update screenshot
@peterhashair peterhashair added the Needs Review PRs that need a code review label Jan 19, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

@peterhashair the capitalized screenshot needs to be updated. It's currently an empty file.
Once tests are passing, that is good to merge.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jan 19, 2022
@peterhashair
Copy link
Contributor Author

I guess that should be good to go

@peterhashair peterhashair merged commit a02ae2f into next_release Jan 20, 2022
@peterhashair peterhashair deleted the m-18640-search branch January 20, 2022 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expandable search field no longer search case insensitive and neither
3 participants