samsaffron/codex/convert filter tips to use dmenu #33876
Conversation
SamSaffron
commented
Jul 27, 2025
- feat: use DMenu for filter tips
- remove trigger
|
|
||
| this.selectedIndex = -1; | ||
|
|
||
| next(() => { |
There was a problem hiding this comment.
schedule("afterRender", ...) for operations on the DOM
|
|
||
| updateValue(value) { | ||
| this.newQueryString = value; | ||
| this.args.updateQueryString(value); |
There was a problem hiding this comment.
name should be something like onChange, what we do with the new value is implementation detail of the parent component
| @inputElement={{this.inputElement}} | ||
| /> | ||
|
|
||
| <DMenu |
| } else { | ||
| data.last_seen_users = true; | ||
| } | ||
| const response = await ajax("/u/search/users.json", { data }); |
There was a problem hiding this comment.
we try catch tags, but not this one
| {{didInsert this.storeInputElement}} | ||
| {{didInsert this.setupEventListeners}} |
There was a problem hiding this comment.
We can probably use {{on "focus" ...}} and others, to remove a lot of code
| this.updateValue(updatedValue); | ||
| this.searchResults = []; | ||
| this.updateResults(updatedValue); |
There was a problem hiding this comment.
We update the queryString twice in 3 lines here. It's a code smell for me. Having this kind of duplicated setters makes it hard to understand the flow
| if (this.searchTimer) { | ||
| cancel(this.searchTimer); | ||
| } | ||
| this.searchTimer = discourseDebounce( | ||
| this, | ||
| this._performPlaceholderSearch, | ||
| filterName, | ||
| valueText, | ||
| tip, | ||
| prefix, | ||
| 300 | ||
| ); |
There was a problem hiding this comment.
This is not debounced. Cancelling a debounce handler before calling it, defeats the purpose of it.
| } | ||
|
|
||
| hideTipsIfNeeded() { | ||
| this.handleBlurTimer = null; |
There was a problem hiding this comment.
I don't think that is necessary
| if (this.handleBlurTimer) { | ||
| cancel(this.handleBlurTimer); | ||
| } |
There was a problem hiding this comment.
cancel will not generate an error on undefined values, so you don't need the if here
| searchTimer = null; | ||
| handleBlurTimer = null; | ||
|
|
||
| @tracked _selectedIndex = -1; |
There was a problem hiding this comment.
I think we should stop using these _xxxx. How is _selectedIndex� more private that handleBlurTimer for example?
| }); | ||
|
|
||
| if (tip?.type) { | ||
| this.activeFilter = filterName; |
There was a problem hiding this comment.
should maybe just be a boolean, we never do anything of the actual filter name.
| if (this.selectedIndex >= 0) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| event.stopImmediatePropagation(); |
| } | ||
|
|
||
| @action | ||
| handleKeyDownTips(event) { |
There was a problem hiding this comment.
That's probably a bigger refactor, and debatable, but having the listeners on the list seems better. You should be able to do exactly the same behavior with this technique.
| const aStartsWith = aName.startsWith(lastWord); | ||
| const bStartsWith = bName.startsWith(lastWord); |
There was a problem hiding this comment.
This is case sensitive, not sure this is what you want here
|
@jjaffeux I will cover all the review items here |
| })); | ||
| } | ||
|
|
||
| /* selecting an item ------------------------------------------------------- */ |
There was a problem hiding this comment.
we can probably remove this comment
| @contentClass="filter-navigation__tips-dropdown" | ||
| > | ||
| <:content> | ||
| {{#if (and this.showTips this.currentItems.length)}} |
There was a problem hiding this comment.
should we just clear currentItems when we hideTips?
| get currentItems() { | ||
| return this.filteredTips; | ||
| } |
There was a problem hiding this comment.
We could use filteredTips directly here, I don't think currentItems is a better name
| @@ -81,6 +124,462 @@ export default class DiscoveryFilterNavigation extends Component { | |||
| this._allowEnterSubmit = !value; | |||
| } | |||
|
|
|||
| @action | |||
| setupEventListeners() { | |||
There was a problem hiding this comment.
you can have the element here setupEventListeners(element) so you don't need the check