UI: Add allocation directory rendering #5873
Conversation
|
I changed the base branch to ETA the Netlify deploy won’t work to check out this work because there’s no Mirage response for it yet. |
This doesn’t work in every instance; sometimes when I refresh there’s a failure. I’ll have to tune the models created to eliminate these occasional problems.
backspace
changed the title
This will surely break under some circumstances; refinement to come.
| const statusIs500 = response.status === 500; | ||
| const bodyIncludes404Text = body.includes('no such file or directory'); | ||
|
|
||
| const translatedCode = statusIs500 && bodyIncludes404Text ? 404 : response.status; |
There was a problem hiding this comment.
I’m awaiting feedback on whether 500-as-404 works similarly across platforms, so this may not be quite ready, but everything else is apart from the custom Mirage scenario that I can remove later.
There was a problem hiding this comment.
I changed this to a TODO as I think it’s fine to leave in for now and return to if the API changes.
There was a problem hiding this comment.
This is gonna be awesome!! 🤩
Generally speaking, this is great work. I had a lot of nits, but all the big stuff is good and in place.
Also apologies for being such a stickler when it comes to CSS, but that's the only way to keep that stuff under control, you know?
| import FSRoute from './fs'; | ||
|
|
||
| export default FSRoute.extend({ | ||
| templateName: 'allocations/allocation/task/fs', |
There was a problem hiding this comment.
It personally trips me up when there is a route that has no template due to something like this, but I'm willing to try it out.
There was a problem hiding this comment.
yeah, I don’t love it either 😐
| return { | ||
| path: decodedPath, | ||
| task, | ||
| isFile: true, |
There was a problem hiding this comment.
Do you think it's worth adding a directoryEntries: [] here just to ensure that the controller prop is always an array?
I'm on the fence about it.
There was a problem hiding this comment.
hmm… I don’t really even like this giant-conditional-in-the-template situation but I feel like isFile adequately protects against any attempt to iterate through directoryEntries. I’d be fine adding it though if you think it’s helpful!
|
|
||
| assert.equal(FS.breadcrumbs.length, 1); | ||
| assert.ok(FS.breadcrumbs[0].isActive); | ||
| assert.equal(FS.breadcrumbs[0].text, 'task-name'); |
There was a problem hiding this comment.
A pretty clear downside to the data-in-the-mirage-handler approach is that you can't reach into the mirage db to get the task-name value out.
There was a problem hiding this comment.
agreed. Hard-coding irks me somewhat but feels okay to me for this minimal case. It’s still possible to convert to a more ORMy setup if you think that’s important though!
|
I appreciate the nits, especially with CSS, which definitely needs a firm hand 😆 I think I’ve addressed your suggestions etc. |
|
I’ll leave the Mirage scenario I noticed that the breadcrumb indentation is off now, as related to the first column of the table: I’ll address that in a separate PR. |
|
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. |
You can try it out with this direct link to the deployment preview.