-
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
Fix navigation via clicking recent allocation row #6087
Conversation
@@ -20,7 +23,7 @@ export default Component.extend({ | |||
|
|||
actions: { | |||
gotoAllocation(allocation) { | |||
this.transitionToRoute('allocations.allocation', allocation); | |||
this.router.transitionTo('allocations.allocation', allocation.id); |
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.
any specific reason to reload the record?
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 found that without the .id
, I got this error:
You didn't provide enough string/numeric parameters to satisfy all of the dynamic segments for route
My belief is that it’s because the router is expecting allocation_id
, but the model only has an id
. Changing the router seemed like Too Much for this small fix.
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.
LGTM
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.
Nice, UX++ I wonder how long that has been broken 😬
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. |
I noticed that the allocation row from job overview had the pointer cursor but clicking outside the allocation id didn’t actually do anything. It looks like the action was written for a controller context, so I changed it to use the router service. I also added
.id
to the transition call because without it I was gettingYou didn't provide enough string/numeric parameters to satisfy all of the dynamic segments for route
. Maybe this is because the router definition is looking forallocation_id
, but I didn’t want to get into changing that.I added a test, which ended up being a set of tests, because of the
moduleForJob
extraction. You can see it failing here (linking to a set of lines intravis-web
appears to be broken 😞) before I committed the fix.