UI: Add phase 1 search #8175
UI: Add phase 1 search #8175
Conversation
This is an ungainly integration with the searchable mixin 😳
I’d prefer for this to be covered by automated tests but it seems like it only half-works… the placeholder in the field disappears with the use of keyDown but the field doesn’t look active. Actually pressing / does work though.
Ember Asset Size actionAs of e4e7c78 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
Has it always been that errors are silently swallowed? Mysterious
With the expanded try, I found that the reason for search only working once was that it was required to use set for these properties, I guess because they’re used by computed properties.
This is still unwieldly but maybe a bit more sensible now.
I’ve adapted the listener from @DingoEatingFuzz’s keyboard event- handling here, obviating the need for ember-keyboard: https://github.com/hashicorp/nomad/pull/8177/files#diff-eb4d61e1e080830b844012c6617c6fa2 The filtering to prevent stealing events from input fields is adapted from here: https://stackoverflow.com/a/4171758
This still means a query will be sent for every search, but if we’re in a route we know has a watched collection, the cached collection will be used.
Since the container now has align-items: center, this is no longer needed.
This isn’t perfect as it only updates the last-fetched time of a watched collection when a search is performed. It also has the cache duration as 5s which is probably too short.
| collectionLastFetched = {}; | ||
|
|
||
| async fetch(modelName) { | ||
| const modelNameToRoute = { |
There was a problem hiding this comment.
Maybe this could be dynamically calculated by examining routes with watchers but that seemed like overkill for this small use.
There was a problem hiding this comment.
I agree. This quick little mapping does the job just fine.
There was a problem hiding this comment.
So happy to see this come together! The designs for it have been gathering dust for long enough.
It'll be nice to get the namespace bug fixed for the beta but not at the expense of this missing the beta altogether, so maybe that should be a follow up PR? I'll leave that up to you.
| const itemModelName = model.constructor.modelName; | ||
|
|
||
| if (itemModelName === 'job') { | ||
| this.router.transitionTo('jobs.job', model.name); |
There was a problem hiding this comment.
I think if this includes the namespace query param then the second bug I noticed goes away. I believe this is already being done in the allocation row template?
| @alias('dataSource.searchString') searchTerm; | ||
|
|
||
| fuzzySearchEnabled = true; | ||
| } |
There was a problem hiding this comment.
This is a nice concise way to separate out search strategies for different types ✨
| collectionLastFetched = {}; | ||
|
|
||
| async fetch(modelName) { | ||
| const modelNameToRoute = { |
There was a problem hiding this comment.
I agree. This quick little mapping does the job just fine.
| const clock = sinon.useFakeTimers({ | ||
| now: new Date(), | ||
| shouldAdvanceTime: true, | ||
| }); |
There was a problem hiding this comment.
Nice! I've always wanted to use this somewhere.
|
|
||
| assert.notOk(PageLayout.navbar.search.field.isPresent); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💯 good test coverage.
|
A GIF of the “search focus blackhole” @DingoEatingFuzz encountered in Chrome:
|
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is scavenging of #7720 that doesn’t use the search endpoint. It introduces a
DataCachesservice so recently-updated collections don’t need to be requeried within a time period (currently a minute) or based on the current route.A GIF of keyboard usage:
I’m submitting it for review with a known bug regarding the use of the escape key while searching. There’s a sort of intermediate state where the field appears to be active but there’s no insertion caret to actually search, pressing enter causes it to become an editable field. I was able to use some manipulation in
openOnClickOrTabto skip that intermediate state when entering the field but I couldn’t quite figure out how to also skip it when pressing escape. I think something will be possible withrun.lateror the like but in the interest of having this be part of the beta, I’m grudgingly accepting this UX annoyance.There’s also a not-quite-bug but less-efficient aspect to the collection caching, which I have noted as a TODO.