UI: Allocation file system explorer: Routing #5866
Conversation
There was a problem hiding this comment.
Code looks good! One little suggestion there, but no biggie. Especially appreciate the testing here.
One question I have is that it looks like you are going to have URLs like:
fs/path%2Fto%2Fa%2Ffolder%2Fsomewhere
instead of:
fs/path/to/a/folder/somewhere
...unless I misread that code.
Was that a specific decision to encode the slashes? You can probably forsee my following query if so, i.e. should we be doing it like vault and consul with non-encoded slashes?
John
P.S. Oh also, should I be seeing the PR preview thing that @backspace added here? It might be my rubbish GH skillsz, but I don't see it?
| model({ path }) { | ||
| return { | ||
| path: decodeURIComponent(path), | ||
| task: this.modelFor('allocations.allocation.task'), |
There was a problem hiding this comment.
Little suggestion, could you just call the task property model? Then you don't need to rename it below?
setupController(controller, model) {
this._super(...arguments);
controller.setProperties(model);
}It's clear from the modelFor argument that it's a task
There was a problem hiding this comment.
I don’t know that this quite reaches convention status, but I’ve referred to this before as a “compound model”. I haven’t seen many conventions around the naming of its components apart from them being named the things that they are. I think it would be somewhat confusing to have the model hook be loading something that contains a path and a model.
I hope some day there’s a more first-class way to do this kind of data loading!
There was a problem hiding this comment.
The other option would be to keep task and not rename it to model in setupController. I think my main point was more not having to rename it than deciding what to call it. I know that some folks prefer having a model in the templates, assumed this was why it was being renamed.
There was a problem hiding this comment.
Ideally I wouldn't have to do model hook shenanigans to pass that param to the controller, but if the param changes without the model changing then the setupController hook doesn't refire.
But I also wouldn't get caught up on this. I expect this route to more or less be entirely rewritten when @backspace works on the file explorer component. Hard to say now what the best way to provide context to the controller is.
There was a problem hiding this comment.
Sweet yeah, gotcha, was just a tiny thing I noticed, no biggie.
| }); | ||
|
|
||
| test('visiting /allocations/:allocation_id/:task_name/fs/:path', async function(assert) { | ||
| const paths = ['some-file.log', 'a/deep/path/to/a/file.log', '/', 'Unicode™®']; |
| @@ -0,0 +1,14 @@ | |||
| {{partial "allocations/allocation/task/subnav"}} | |||
There was a problem hiding this comment.
Something for the future, do you have any interest in removing the use of this partial? As it looks like partials will be deprecated. It seems like it could be replaced by a simple component.
There was a problem hiding this comment.
I'm quite lackadaisical when it comes to dealing with deprecations. Of course it makes sense for these to become components eventually, and I won't stand in anyone's way who wants to do that work now. I just tend to wait until I have to 😛
|
Regarding the URLs, slashes and whatnot will be supported in the URL. Example screenshot:
From the code, you're seeing two things.
And, apologies in advance for this sounding like a copout, I wouldn't get too hung up on this either. This PR is just to set us up to work in parallel. Before this code makes it to |
That's a good question, and one that @backspace is more prepared to answer. My hunch is that it isn't building because this PR isn't against master? |
|
Cool thanks for the info, I was only because I'd just considered doing the %2F thing in consul and maybe think that would have been the better choice. I did a load of custom stuff in the end, but that could potentially be removed if we were ok with it. Hey and definitely I know it's never ever any copout's, its not fun dealing with things like this stuff so if theres a preference to avoid them some other way thats cool in my book. Sorry if it sounds like I'm asking things a bit early 😬 ! I find myself again longing for the :reverse_bushes: emoji in GH! 😂 I'll probably dip out of these a lot more now you have a good thing going. |
That’s my theory too, though the Netlify settings don’t indicate any such limitation:
Last time I wrote to Netlify support they were very eager to respond, though the answer was unfortunate (no way to restrict deployments based on branch name), so I’ll check in with them on this. |
|
Gonna merge this now. The Netlify thing may strike again on future PRs for this project though if our theory holds true. |
|
Re Netlify, I got a response from support and changed the settings to deploy the base branch and now it’s working:
I could change “Branch deploys” to just always deploy for all branches but I feel like manually updating it when we have this kind of long-running branch is fine for now, as I’m still wary of adding more deployment-failure noise to non-UI PRs. |
|
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 lays out the foundation for building out the alloc fs explorer. This allows us to work on the explorer component and file view components in parallel.