-
Notifications
You must be signed in to change notification settings - Fork 280
feat(ui5-shellbar): add preventable search field clear event #12227
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
Looks good overall, just two comments:
|
43cd0d7
to
6114c0e
Compare
packages/fiori/src/ShellBar.ts
Outdated
@@ -661,7 +680,7 @@ class ShellBar extends UI5Element { | |||
this._detachSearchFieldListeners(e.target as HTMLElement); | |||
return; | |||
} | |||
if (!isPhone() && !this.search?.value) { | |||
if (!isPhone()) { |
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 covers a case in how the search opens/collapses. Instead, we should extend it with || !this.showSearchField, so that we also cover the case when the search field is explicitly closed.
To test this, you can type something in the filed and press the search icon - it shouldn't close the filed, the users will instead trigger a search.
The logic here ensures:
- On mobile, the search opens on its own (we don’t interfere).
- If there’s already a value, the search event is responsible for triggering the search (we don’t interfere).
- (the new condition) If the field is closed, we must open it regardless.
It's not well documented, lets add a comment above to have this info in mind when making changes in future, e.g,:
// Decide when to toggle the search field:
// - On mobile, the search opens on its own (we don’t interfere).
// - If there’s already a value, onSearch is responsible for triggering the search (we don’t interfere)
// - If the field is closed, we must open it regardless.
if (!isPhone() && (!this.search?.value || !this.showSearchField)) {
this.setSearchState(!this.showSearchField);
}
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.
Minor (optional): We could also flip the condition for readability, e.g.:
if (isPhone() || (this.search?.value && this.showSearchField)) {
return;
}
this.setSearchState(!this.showSearchField);
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.
Thanks for the well-done review! I really didn’t know about this!
The cancel button now clears the search field value by default. Previously, the search field value was preserved. To maintain the previous behavior, listen for the search-field-clear event and call preventDefault(). - Add search-field-clear event with preventDefault support - Change default behavior to clear search field value on cancel - Separate clearing behavior from closing behavior - Add comprehensive test coverage for both scenarios Jira: BGSOFUIPIRIN-6909
6114c0e
to
02c0ad0
Compare
🎉 This PR is included in version v2.14.0-rc.7 🎉 The release is available on v2.14.0-rc.7 Your semantic-release bot 📦🚀 |
The cancel button now clears the search field value by default. Previously, the search field value was preserved. To maintain the previous behavior, listen for the
search-field-clear
event and callpreventDefault()
.search-field-clear
event withpreventDefault
supportJira: BGSOFUIPIRIN-6909