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

feat(dropdown): add case sensitivity setting for search #1101

Merged

Conversation

aexvir
Copy link
Contributor

@aexvir aexvir commented Oct 17, 2019

Description

ignoreCase was only used for affecting the label creation. This MR makes the filtering respect that setting, for both searches, on text and value.

Instead or reusing the same variable, as that would introduce a breaking change where none of the current users will have case insensitive search, this MR adds a new setting that allows setting the case sensitivity for the item filtering

Testcase

before: https://jsfiddle.net/3we8nb7j/
after: https://jsfiddle.net/ye1hg6c4/

Closes

#932

@lubber-de lubber-de added state/awaiting-reviews Pull requests which are waiting for reviews type/feat Any feature requests or improvements lang/javascript Anything involving JavaScript labels Oct 17, 2019
@lubber-de lubber-de added this to the 2.8.0 milestone Oct 17, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

Your code works, but as ignoreCase is false by default, no existing application search dropdowns will work anymore without refactoring them by adding ignoreCase:true. (see your own jsfiddle example)
Although 2.8.0 allows breaking changes, this kind of change would be a bit too unexpected and i believe dropdowns are used a lot everywhere.
My suggestion is to make this an additional setting ignoreSearchCase (default true) , so ignoreCase will only work for the labels as before
@hammy2899 @exoego @prudho @ColinFrick Your opinions? (before @aexvir is going to change it)

@lubber-de lubber-de added the state/awaiting-docs Pull requests which need doc changes/additions label Oct 17, 2019
@y0hami
Copy link
Member

y0hami commented Oct 17, 2019

I agree with @lubber-de, keeping ignoreCase as backwards compatible is the way to go because changing its behavior can't be justified for the impact it would have on users. Also adding this setting allows the user more control over the search case.

@aexvir
Copy link
Contributor Author

aexvir commented Oct 17, 2019

Yep, totally agree

@aexvir aexvir force-pushed the aexvir/dropdown-search-case-sensitivity branch from d2c0ef4 to 4611ce5 Compare October 17, 2019 17:28
@aexvir aexvir requested a review from lubber-de October 17, 2019 17:31
@aexvir aexvir changed the title [Dropdown] Respect case sensitivity setting on filtering [Dropdown] Add case sensitivity setting for filtering Oct 17, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Oct 18, 2019
@y0hami y0hami changed the title [Dropdown] Add case sensitivity setting for filtering feat(dropdown): add case sensitivity setting for search Oct 18, 2019
@y0hami y0hami merged commit c1d5119 into fomantic:develop Oct 18, 2019
@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Oct 19, 2019
@lubber-de
Copy link
Member

Docs added by fomantic/Fomantic-UI-Docs#161

y0hami pushed a commit to fomantic/Fomantic-UI-Docs that referenced this pull request Oct 19, 2019
@y0hami
Copy link
Member

y0hami commented Oct 24, 2019

@all-contributors please add @aexvir for code

@allcontributors
Copy link
Contributor

@hammy2899

I've put up a pull request to add @aexvir! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants