Skip to content
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

Merged

Conversation

Techn1x
Copy link
Contributor

@Techn1x Techn1x commented Dec 3, 2022

Aiming to help out with the ember v4 work where I can #444

classic syntax -> native class
curly brackets -> angle brackets

@Techn1x Techn1x changed the title refactor: native class dummy app files refactor: octane dummy app files Dec 3, 2022
Comment on lines -8 to -13
setupController(controller, users) {
let { meta = {} } = users;
controller.set('model', users);
controller.set('previousPage', meta.previous);
controller.set('nextPage', meta.next);
},
Copy link
Contributor Author

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.

Comment on lines 17 to +20
keyForAttribute() {
return this._super(...arguments);
},
});
return super.keyForAttribute(...arguments);
}
}
Copy link
Contributor Author

@Techn1x Techn1x Dec 3, 2022

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) {}
Copy link
Contributor Author

@Techn1x Techn1x Dec 3, 2022

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

@Techn1x
Copy link
Contributor Author

Techn1x commented Dec 3, 2022

cc @patocallaghan

Let me know if there's anything I can help you with

@patocallaghan
Copy link
Collaborator

Thanks @Techn1x. Will review later tonight 🙌

Copy link
Collaborator

@patocallaghan patocallaghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @Techn1x 🙌

@patocallaghan patocallaghan merged commit 9be78a4 into adopted-ember-addons:master Dec 6, 2022
@patocallaghan patocallaghan mentioned this pull request Dec 6, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants