-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Clients filtering #5318
UI: Clients filtering #5318
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.
Some comments from just reading the code. Looks great!
I'm checking out the branch locally now but I might run out of time for UX feedback, so here's a ✅for now: ✅
// Remove any invalid node classes from the query param/selection | ||
scheduleOnce('actions', () => { | ||
this.set('qpClass', serialize(intersection(classes, this.get('selectionClass')))); | ||
}); |
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 all seems correct, but I'm a little wary of side effects in computed properties. What do you think about moving these QP clean-ups into a method which is called from the route's setupController
hook, after the model is set?
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 definitely like that better, but it won't capture the case where via blocking queries the underlying data changes and invalidates query params (e.g., clients change node class for some reason).
Although... maybe I could add some new hook to routes that have Watchers... 🤔
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.
Okay, I did some light exploration on this idea and I think it has legs. I'd prefer to do it in a follow up PR though.
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'd completely forgotten about blocking queries when I suggested it. Makes sense to explore post-merge.
}); | ||
}); | ||
|
||
function testFacet(label, { facet, paramName, beforeEach, filter, expectedOptions }) { |
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'm a big fan of these test generating functions 👍
if (onlyIneligible && node.get('isEligible')) return false; | ||
if (onlyDraining && !node.get('isDraining')) return false; | ||
|
||
return true; |
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 is clear and easy to read!
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 part three of
threefour for faceted search in the Nomad UI: #5235Faceted search on the Clients page introduces quick filter options for finding clients.
NodeClass
datacenter
Face selections are persisted in query params in order to be shareable and bookmarkable.
Reviewers note: I did some light refactoring of helpers that were used in the
jobs.index
controller. As for test helpers, I wanted to generalize thetestFacet
helper, but the two implementations are just different enough that I didn't think it would make sense. It's not exactly core code and I don't think it will be touched again any time soon aside from modernization during the ember upgrade.