Hosting Command Palette: Add test cases for filtering logic #85838
Conversation
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
| const beforeSeparator = 'Open hosting configuration'; | ||
| const afterSeparator = 'cache hosting manage'; | ||
| const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator; | ||
| expect( commandFilter( value, search ) ).toBe( 0.5 ); |
There was a problem hiding this comment.
@sejas I added test for this and the function for the filter does return 0.5 as expected so technically it does what the prop describes (return the number between 0 and 1) but it does not change the ranking. I think this indicates that there must be an issue with cmdk. What are your thoughts?
There was a problem hiding this comment.
Just to add, I think we should also some tests to check the rendering of the commands when you are rending an actual array since these are fairly simple and only check if the matching works as expected
There was a problem hiding this comment.
@sejas I added test for this and the function for the filter does return 0.5 as expected so technically it does what the prop describes (return the number between 0 and 1) but it does not change the ranking. I think this indicates that there must be an issue with cmdk. What are your thoughts?
I think we should report the issue to cmdk to fix it. I think they didn't create a release almost a year ago, so it could be they implemented it and didn't release it yet.
There was a problem hiding this comment.
Just to add, I think we should also some tests to check the rendering of the commands when you are rending an actual array since these are fairly simple and only check if the matching works as expected
Yes! That would be fantastic. If it's not too complicated, we could create a fake array of commands, and then search for something and check if it's in the array or/and if it's a first result.
There was a problem hiding this comment.
Sounds good, I will report the issue to them and work on the tests today! Thanks for the review!
There was a problem hiding this comment.
@sejas I search the cmdk repo and found the issue with filtering was reported here: pacocoursey/cmdk#118
There is current a PR in progress to fix the issue: pacocoursey/cmdk#182
Related to https://github.com/Automattic/dotcom-forge/issues/4952
Proposed Changes
This PR adds tests for
use-command-filterhook to confirm that it returns what is expected:0, when there is no command match;0.5, when there is a match in thesearchLabel1, when there is a match in thelabelTesting Instructions
yarn test-client -- client/components/command-palette/test/use-command-filter.tsxPre-merge Checklist