-
Notifications
You must be signed in to change notification settings - Fork 65
Fix placeholdersearch #710
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🌯
bors merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bidoubiwa i missed the timing here, but I really like to left some notes 🌮
expect(searchParams.limit).toBe(0) | ||
}) | ||
|
||
test('Adapt SearchContext placeholderSearch set to false', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test has the same description as the first one :)
pagination, | ||
defaultFacetDistribution, | ||
placeholderSearch: !options.placeholderSearch, // true by default | ||
placeholderSearch: options.placeholderSearch !== false, // true by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is protected by type? Because if the user add 123
, 0
in this option, the option will be true
:)
await meilisearchClient.index('movies').waitForTask(documentsTask.uid) | ||
}) | ||
|
||
test('Test placeholdersearch set to false', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about describing what the test should be expecting?
Responds with no hits when placeholderSearch is falsy
.
expect(hits.length).toBe(5) | ||
}) | ||
|
||
test('Test placeholdersearch set to true', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo here (the setup use placeholderSearch: false
)
Pull Request
What does this PR do?
Fixes #709
placeholderSearch
is suppose to not showcase any search result when the query is empty. It now correctly does what it is suppose to do. It is also improved as it will now avoid to do a search request to Meilisearch.