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

Change the filtervalue to lowercase #14786

Merged
merged 2 commits into from May 1, 2019

Conversation

@Jackie6
Copy link
Contributor

commented Apr 3, 2019

Description

Fix #13250

How has this been tested?

  • local test
  • browser test

Types of changes

Change the filterValue to lower case

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Apr 3, 2019

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

I'm not super familiar with its implementation, but should this be incorporated into what getFilterMatcher is intended to serve? cc @notnownikki re: #10138

@draganescu
Copy link
Contributor

left a comment

I think this should be treated in the function, not as a quickfix in the function's param. If we look at the getFilterMatcher function this part appears to be the problem:

// If the term's name contains the filterValue, or it has children
// (i.e. some child matched at some point in the tree) then return it.
if ( -1 !== term.name.toLowerCase().indexOf( filterValue ) || term.children.length > 0 ) {
	return term;
}

this condition lowercases the term but allows the filterValue to remain as is. I think it should be::

// If the term's name contains the filterValue, or it has children
// (i.e. some child matched at some point in the tree) then return it.
if ( -1 !== term.name.toLowerCase().indexOf( filterValue.toLowerCase() ) || term.children.length > 0 ) {
	return term;
}

Does this sound right? @Jackie6 @youknowriad

@Jackie6 Jackie6 force-pushed the Jackie6:update/terms-filter branch from 155d8c4 to 981e0b4 Apr 24, 2019

@Jackie6

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Hi @draganescu thanks for your review. I think you are right. It is better to put toLowerCase() functions together.

Remove the space between paratheses
Co-Authored-By: Jackie6 <541172791@qq.com>

@draganescu draganescu merged commit 56e6d77 into WordPress:master May 1, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.