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

[ Feature ] Support Assets encoded in Data URI #1189

Merged
merged 1 commit into from Sep 22, 2018

Conversation

Projects
None yet
2 participants
@TechQuery
Copy link
Contributor

TechQuery commented Sep 11, 2018

  • [ Add ] Support Assets encoded in Data URI
  • [ Optimize ] Simplify core logic of the loader

Resolves #1188

[ Add ] Support Assets encoded in DataURI
[ Optimize ]  Simplify core logic of the loader

@TechQuery TechQuery changed the title [ Add ] Support Assets encoded in DataURI [ Feature ] Support Assets encoded in Data URI Sep 11, 2018

@starwed

This comment has been minimized.

Copy link
Member

starwed commented Sep 13, 2018

Hey, thanks for the PR! I might not be able to get to looking over it until this weekend.

One thing to note is that using the Promise object would break compatibility with IE 9 - 11 completely which Crafty nominally supports. (But for many reasons, it might be time to just break that chain.)

@TechQuery

This comment has been minimized.

Copy link
Contributor Author

TechQuery commented Sep 15, 2018

@starwed
To import a Promise polyfill from CDN is so easy now, and it's good for making the code simpler & stronger.

At the same time, I also suggest to refactor this project with ECMAScript 6+ & Babel 7, it will make the core logic clearer.

@starwed starwed merged commit a480308 into craftyjs:develop Sep 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@starwed

This comment has been minimized.

Copy link
Member

starwed commented Sep 22, 2018

Looks good, merged! Thanks for the PR.

I made some non-functional tweaks (updating qunit in our package.json to match the version you'd manually updated to, upgrading our jshint rules to allow es6, and some minor formatting to whitespace.) Merge commit here.

I also suggest to refactor this project with ECMAScript 6+ & Babel 7, it will make the core logic clearer.

I think I agree with this (and see #1106), at least with moving to ES6 where possible. I updated our linting rules to allow es6.

@TechQuery

This comment has been minimized.

Copy link
Contributor Author

TechQuery commented Sep 25, 2018

@starwed When is this feature published as a new version? A product of my company is based on it.

@starwed

This comment has been minimized.

Copy link
Member

starwed commented Sep 30, 2018

@starwed When is this feature published as a new version? A product of my company is based on it.

Each commit to develop pushes a build to the Crafty-Distro repository, so if you just need the crafty.js file that can work.

A full release (including pushing to npm, etc) generally happens when there's a useful enough set of changes to warrant it, and there aren't any additional breaking changes planned that would cause churn. In this case the fix includes breaking changes (using ES6 functionality), so I'd want to make some related changes before doing a full release.

If publishing to the npm is important, I can probably publish a beta/alpha build? Let me know if that's useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment