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

Convert addon to TypeScript & add glint #581

Merged
merged 35 commits into from
Sep 21, 2024

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Sep 20, 2024

This PR converts the addon to TS and adds glint support

Copy link

netlify bot commented Sep 20, 2024

Deploy Preview for ember-sortable canceled.

Name Link
🔨 Latest commit 2bee092
🔍 Latest deploy log https://app.netlify.com/sites/ember-sortable/deploys/66eed24424d7f70008da6942

if (!scrollParent || scrollParent === document.body) {
scrollParent = document;
}
return position === 'fixed' || scrollParent;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while TS converting removed position === 'fixed' because it makes no sense... the return must be a HTMLElement or Document otherwise code is not correctly in scroll-container.ts because he needs always a dom element https://github.com/adopted-ember-addons/ember-sortable/pull/581/files#diff-49b1162a2852f7a45dc24d5027bc3ea12f95d37caef8c2b020f7709803bb01a6R24

const sortedItems = this.sortedItems;
const itemElement = sortedItems[moves[0].fromIndex].element;
Copy link
Contributor Author

@mkszepp mkszepp Sep 20, 2024

Choose a reason for hiding this comment

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

this code was never correct... moves is a array of number array

instance.announcer.parentNode.removeChild(instance.announcer);
}
instance.removeEventListener();
instance.sortableService.deregisterGroup(instance.groupName, instance);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deregisterGroup has no 2nd parameter, so i have removed in TS

Comment on lines +63 to +65
"@embroider/macros": "^1.16.6",
"@glimmer/env": "^0.1.7",
"rsvp": "^4.8.5"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this dependencies were always missed 😁, but used inside addon

@mkszepp
Copy link
Contributor Author

mkszepp commented Sep 21, 2024

ci is now greeen 🎉...

For ember-canary ( ember 6) there were necessary to make some deprecation cleanup (template location and disable prototype extension)... this test fails atm also on master.

@NullVoxPopuli NullVoxPopuli merged commit 1572469 into adopted-ember-addons:master Sep 21, 2024
22 of 23 checks passed
@github-actions github-actions bot mentioned this pull request Sep 21, 2024
@mkszepp mkszepp deleted the convert-to-ts branch September 21, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants