-
-
Notifications
You must be signed in to change notification settings - Fork 137
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
refactor: octane dummy app files #474
refactor: octane dummy app files #474
Conversation
setupController(controller, users) { | ||
let { meta = {} } = users; | ||
controller.set('model', users); | ||
controller.set('previousPage', meta.previous); | ||
controller.set('nextPage', meta.next); | ||
}, |
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.
One change I've made that should be functionally the same - I've created a controller for this route and moved the previous/next properties there.
keyForAttribute() { | ||
return this._super(...arguments); | ||
}, | ||
}); | ||
return super.keyForAttribute(...arguments); | ||
} | ||
} |
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.
Not sure why keyForAttribute is defined here when it just calls its super, but I've left it alone in case it's there for a reason (I'm not super familiar with the EmbeddedRecordsMixin)
export default JSONAPIAdapter.extend(AdapterFetch); | ||
export default class extends JSONAPIAdapter.extend(AdapterFetch) {} |
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 will conflict with the removal of AdapterFetch
#464
But whoever merges last can handle the conflct
Let me know if there's anything I can help you with |
Thanks @Techn1x. Will review later tonight 🙌 |
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.
Looks great @Techn1x 🙌
Aiming to help out with the ember v4 work where I can #444
classic syntax -> native class
curly brackets -> angle brackets