-
Notifications
You must be signed in to change notification settings - Fork 516
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
fix(searchbox): fix usage of custom reset template #2585
Changes from all commits
e0057c3
d9495ac
4e4bdfa
fdd8eec
e2feed7
cce5033
dedbcb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,7 +221,7 @@ describe('searchBox()', () => { | |
widget.init(defaultInitOptions); | ||
|
||
// Then | ||
expect($('button[type="reset"]')[0].style.display).toBe('none'); | ||
expect($('.ais-search-box--reset-wrapper')[0].style.display).toBe('none'); | ||
}); | ||
|
||
it('should be shown when there is a query', () => { | ||
|
@@ -233,7 +233,9 @@ describe('searchBox()', () => { | |
simulateInputEvent('test', 'tes', widget, helper, state, container); | ||
|
||
// Then | ||
expect($('button[type="reset"]')[0].style.display).toBe('block'); | ||
expect($('.ais-search-box--reset-wrapper')[0].style.display).toBe( | ||
'block' | ||
); | ||
}); | ||
|
||
it('should clear the query', () => { | ||
|
@@ -243,7 +245,7 @@ describe('searchBox()', () => { | |
simulateInputEvent('test', 'tes', widget, helper, state, container); | ||
|
||
// When | ||
$('button[type="reset"]')[0].click(); | ||
$('.ais-search-box--reset-wrapper')[0].click(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the button is still type reset right? Not sure from this change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. The problem is that it is in the template, so we cannot rely on what's inside it for the tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the source of the bug. If you were to use an emoji, there would not be a button (with type=reset) so instead the event listener is on the outer span. |
||
|
||
// Then | ||
expect(helper.setQuery.called).toBe(true); | ||
|
@@ -277,7 +279,7 @@ describe('searchBox()', () => { | |
widget.init(defaultInitOptions); | ||
|
||
// Then | ||
expect($('button[type="reset"]').length).toEqual(0); | ||
expect($('.ais-search-box--reset-wrapper').length).toEqual(0); | ||
}); | ||
}); | ||
|
||
|
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.
We could probably reuse this command from the
dev.sh
script.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 do you mean? This line was extracted from
dev.sh
.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.
I mean in the
dev.sh
we can doyarn dev-novel
instead of having the same command in two different places.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.
That's not a big improvement though. Let's iterate on that later.