UI: Recent allocs + job allocs view #4529
Conversation
There was a problem hiding this comment.
Nice! Made a few comments but mostly to help me understand stuff.
The only thing I did notice when playing with it was that if you do an allocation search and nothing comes up, there is no way to undo your search (apart from removing the query param from the url) as the search bar disappears if you get no results.
Plus I've just noticed in the screengrab that maybe the text shouldn't be 'no allocations have been placed' maybe it should be like 'No allocations meeting your search query' or just 'No results' or something?
Anyway, if you want to do that as a separate PR happy to approve this one, just let me know!
ui/app/templates/clients/client.hbs
Outdated
| @@ -101,7 +101,7 @@ | |||
| <th class="is-narrow"></th> | |||
| {{#t.sort-by prop="shortId"}}ID{{/t.sort-by}} | |||
| {{#t.sort-by prop="modifyIndex" title="Modify Index"}}Modified{{/t.sort-by}} | |||
| {{#t.sort-by prop="name"}}Name{{/t.sort-by}} | |||
| {{#t.sort-by prop="createIndex" title="Create Index"}}Created{{/t.sort-by}} | |||
There was a problem hiding this comment.
Not a lot of background around Nomad specifically, are names not useful for folks?
There was a problem hiding this comment.
Not really. Names for allocations are a function of task group name and count. They end up looking like task-group-name.[3].
| .slice(0, 5) | ||
| ), | ||
| }); | ||
| }), |
There was a problem hiding this comment.
Just about of interest, what's the reason for the PromiseArray? I'm guessing the allocations might not be loaded yet when you ask to sort them or something? If you have a sec would be good to get an understanding, no prob if not.
I had a quick look at some other sorting you are doing and none of them use this, are allocations different or is this something you are going to be moving to across the board?
P.S.(edit) Sorry funny the way GH does this, the preview from GH doesn't help here 😄 probably need to view a few lines up also for this to make sense!
There was a problem hiding this comment.
Actually just noticed also, looks like job is an attribute on the component (job="job") and then you are reaching up into the job to get the allocations. Just an offshoot, would an alternative be allocations={{job.allocations}} so you are sending the data down? Not sure if that is workable for you or not
There was a problem hiding this comment.
If it isn't clear, the PromiseArray object makes it so the returned value always has an array-like interface before and after the promise resolves.
This isn't important elsewhere because RecordArray (either from store.findAll or model.get('asyncHasMany') are used in templates.
Here, additional transformations need to happen (sortBy, reverse, slice).
If the promise isn't resolved yet, this breaks because it's not an array yet. Simply doing this.get('job.allocations').then(allocations => allocations.transformations) also doesn't work because the computed property now returns a promise. The template can't do anything with the promise and the computed property doesn't update when the promise resolves.
By using a PromiseArray, I can do the promise control flow while also ensuring an Array interface as well as a template re-render when the promise resolves.
There was a problem hiding this comment.
I could use allocations=job.allocations, and that may be better. Specifically because of the principle of least authority. I chose to use job=job to make all the various job-page/parts components feel similar.
I don't think this violates data-down actions-up because it isn't reaching up into a higher viewer (e.g., allocations: alias('parentView.job.allocations')). Instead, a larger than necessary piece of data is being provided to the component and the component is reaching down in that data.
I'm willing to talk about this more if you wish. I know it's pretty nuanced.
There was a problem hiding this comment.
Hey, thanks for the further detail.
I'm willing to talk about this more if you wish. I know it's pretty nuanced.
It's fine I'm not fussed, just thought I'd highlight a alternative approach if it helps. If you decide to go that route at some point and you want to chat then quite happy to, no prob if not.
Back to the PR - Ping me when you tweak the search thing so you can undo your search when there are no results, and I can give it another once over.
Thanks,
|
Massive aside here, but I noticed this:
..and I'd not noticed them before when running your tests. I tried on master also and they are there aswell so maybe I've just not noticed them. assuming you get these also? One of them is related to allocations somehow, don't know if they effect anything or not but thought I'd mention in case. I also get a tonne of deprecation notices from plugins/addons when running the Consul tests, so no biggie if they aren't affecting anything. |
|
I'm aware of those warnings in tests. They are all benign, but I should eventually clean them up. Always appreciate your thoroughness in reviews! |
DingoEatingFuzz
force-pushed
the
f-ui-job-overview-allocs
branch
2 times, most recently
from
ec9d27e to
0e76c5b
Compare
|
@johncowen Good catch on the search box disappearing when the empty message is shown. I had all the code in place to keep it around, but then used the wrong variable in the I fixed that and added a test for it. Everything should be in good shape now. |
Since there are no recent allocations, those alloc need to be watched
DingoEatingFuzz
force-pushed
the
f-ui-job-overview-allocs
branch
from
0e76c5b to
32c321a
Compare
|
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. |
In an effort to make it faster to get to allocations, this brings two new features to the job detail page.
This brings the UI job detail view into better alignment with the CLI.