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

Move polyfill from a module import to "native" Ember.String.isHTMLSafe #2

Merged
merged 5 commits into from
Mar 3, 2017
Merged

Move polyfill from a module import to "native" Ember.String.isHTMLSafe #2

merged 5 commits into from
Mar 3, 2017

Conversation

Panman82
Copy link
Contributor

@Panman82 Panman82 commented Mar 3, 2017

This largely comes from ember-getowner-polyfill and ember-assign-polyfill. Also includes ember-cli updates and added test scenarios (separate commits).

The current way of importing the function still works but throws a deprecation message so it should be ok to release as 1.0.2.

Note, ember init wanted to change the order of a few things in package.json. I've found it best to allow it, makes it easier to update down the road..

Resolves #1

@workmanw workmanw self-requested a review March 3, 2017 12:01
Copy link
Owner

@workmanw workmanw 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 to me! Thank you.

@workmanw
Copy link
Owner

workmanw commented Mar 3, 2017

@rwjblue @kellyselden Would either of you have any interest in reviewing since you commented on #1? Seems pretty straightforward to me.

Copy link

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Some minor tweaks/suggestions, but looks good overall to me.

LICENSE.md Outdated
@@ -1,6 +1,6 @@
The MIT License (MIT)

Copyright (c) 2016
Copyright (c) 2017
Copy link

Choose a reason for hiding this comment

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

ember-cli made a mistake here (and we are revertting the update to the LICENSE.md blueprint).

@Panman8201 - Can you change this to 2016-2017 instead of just updating to 2017?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually manually made that change, not ember-cli's fault! :) I'll change it to your suggestion.

README.md Outdated
## Compatibility

This addon is tested against each minor ember version starting with 1.10 through 2.5. Additionally tested against,
`ember-stable`, `ember-beta`, and `ember-canary`. A complete list can be found in the [`ember-try.js](https://github.com/workmanw/ember-string-ishtmlsafe-polyfill/blob/master/config/ember-try.js) config.
This addon is tested against each minor ember version starting with 1.10 through 2.8
Copy link

Choose a reason for hiding this comment

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

s/ember/Ember/

@workmanw
Copy link
Owner

workmanw commented Mar 3, 2017

@rwjblue Thank you!

@Panman82
Copy link
Contributor Author

Panman82 commented Mar 3, 2017

So this brings up an interesting question, normally I'd make the suggested changes and squash, but since there are multiple unique commits here I can't do that.?. Or can I say squash "these two separate commits"?? (Not a git expert just yet.) Or should I just ignore this a push another commit for "PR review changes"?

@workmanw
Copy link
Owner

workmanw commented Mar 3, 2017

You can absolutely do that. It may involve multiple steps though. The way I would do it is to check out the base commit you want to squash with, then cherry-pick your changed commit on top, squash those two together. Then rebase the rest of the commits back on top of that new tree. Does that make sense?

It's perfectly fine to also just push a 5th commit that is "PR review changes". This addon doesn't have, and likely won't have, a high volume of commits. A few extra is perfectly fine with me :)

@Panman82
Copy link
Contributor Author

Panman82 commented Mar 3, 2017

Ug... I tried but failed, had to reset a couple times. Just pushed the commit separately, looks like Travis is running tests now.

@workmanw workmanw merged commit 386a91f into workmanw:master Mar 3, 2017
@workmanw
Copy link
Owner

workmanw commented Mar 3, 2017

Thanks again! I'm going to get a new version of this released later today. Probably go to 1.1.0 for this change? Does that seem right? I'm not a semver expert, but this feels like a minor change to me.

@Panman82 Panman82 deleted the true-polyfill branch March 3, 2017 15:51
@Panman82
Copy link
Contributor Author

Panman82 commented Mar 3, 2017

No problem! I try to contribute where/when I feel that I'm able. 😄

Yeah, 1.1.0 seems appropriate. Significant changes internally but still backwards compatible. The deprecation states it'll be around until 2.0.0, so that's sill valid.

@kellyselden
Copy link

Looks like it's working great kellyselden/ember-awesome-macros#256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants