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

Support Ember 3.13 (redux) #668

Merged
merged 3 commits into from
Oct 29, 2019

Conversation

jrjohnson
Copy link
Contributor

@jrjohnson jrjohnson commented Oct 20, 2019

Re-bases and Supersedes and closes #662 and #653

I couldn't figure out a practical way to keep helping move this along in the original branch since it needed a rebase so I created a new one. Hope that is ok @offirgolan. I believe this is the last thing needed to support Ember 3.13 moving forward.

@jrjohnson jrjohnson mentioned this pull request Oct 21, 2019
1 task
@markusm7
Copy link

Are there plans on merging this anytime soon? @offirgolan

addon/validations/factory.js Outdated Show resolved Hide resolved
@jrjohnson
Copy link
Contributor Author

@offirgolan tests are passing 🎉

@@ -1040,7 +1040,8 @@ module('Integration | Validations | Factory - General', function(hooks) {
assert.equal(child.get('validations.errors.length'), 1);
});

test('call super in validations class with no super property', function(assert) {
// https://github.com/offirgolan/ember-cp-validations/pull/656
skip('call super in validations class with no super property', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not want to address this before moving forward here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decision from @offirgolan was to skip for now. At this point getting this updated for Ember 3.13 is getting past critical as it's a blocker for many apps to provide feedback on the Octane beta.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay... I would just be worried about apps not working correctly if we skip this test. 🤔

Copy link
Contributor Author

@jrjohnson jrjohnson Oct 28, 2019

Choose a reason for hiding this comment

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

Me too, I don't actually know what any of this stuff does! I'm just playing referee to get the approved path merged. I'm happy to investigate and do more, but getting this merged has been a very long process so far. Makes me hesitant to add any more complexity that could delay it further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Turbo87 skipping as this is already broken on the latest beta. Will have to circle back and figure out a good solution for this.

@offirgolan offirgolan merged commit 369f080 into adopted-ember-addons:master Oct 29, 2019
@offirgolan
Copy link
Collaborator

Released in v4.0.0-beta.10. Thank you for seeing this through @jrjohnson.

@jrjohnson
Copy link
Contributor Author

Yay! Thanks!!

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.

None yet

5 participants