search

mitchlloyd / ember-screen

A screen size service for Ember
MIT License
50 stars 10 forks source link

Update examples to use modern syntax 📄 #15

Closed gilest closed 2 months ago

gilest commented 3 months ago

Noticed in a few apps I maintain that their screen services are using the old Service.extend syntax for only their ember-screen service.

Suspect this is because there are no examples of the Native class w/ decorator version of this usage.

Readme rendered

Tested in a running app and also changed the "dummy" app's service to use this syntax. Tests pass locally.

mitchlloyd commented 2 months ago

Thanks for the contribution @gilest! It's been awhile since I've touched this project. Any idea what versions of Ember this would break?

I'm assuming it needs a major version release but I can take a look next week.

gilest commented 2 months ago

Thanks for the contribution @gilest! It's been awhile since I've touched this project. Any idea what versions of Ember this would break?

I think the Octane edition (native classes) arrived in 3.15, but I think the service being in native class syntax might work in even earlier versions of ember.

Moot point though, I reckon, because not many addons even test for versions prior to 3.28.

I'm assuming it needs a major version release but I can take a look next week.

I doubt this will break many if any apps but, yeah – suggest a major and support >= 3.28.

mitchlloyd commented 2 months ago

After a closer look, there are no breaking changes here. The updated app/services/screen.js file is only used for testing.

My new understanding it that this PR is primarily a documentation update along with some test refactoring to update syntax. Because there's no production code change I'm merging this without a release. But @gilest let me know if you need a release for any reason.

gilest commented 2 months ago

Thanks @mitchlloyd

You're right - no need for a release from my perspective