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

moved popup and show to constructorProperties #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cesine
Copy link
Contributor

@cesine cesine commented Feb 10, 2014

for deprecation warning Confirm.show should be moved to constructorProperties

i was getting this warning in my app, so in my app i had a customized copy of confirm.reel where i moved show and popup to the constructorProperties, and the deprecation warnings went away, but there were never any deprecation warnings with the test suite in matte, maybe i would need to add a test to tease it out... or maybe the example of Confirm.show that i followed is no longer normal, and we should be using an instance ie confirm.show?

tests before the changes (chrome 32.0.1700.107)

[run all]300 specs, 40 failures in 22.595s @ 0:37:23Finished at Mon Feb 10 2014 00:37:23 GMT-0500 (EST)

tests after the changes (chrome 32.0.1700.107)

300 specs, 40 failures in 27.511s @ 0:49:42Finished at Mon Feb 10 2014 00:49:42 GMT-0500 (EST)

for deprecation warning  Confirm.show should be moved to constructorProperties
@cesine
Copy link
Contributor Author

cesine commented Feb 10, 2014

the more i think about this one, the more i think its just because i was using Confirm.show statically, and maybe it's not supposed to be used statically.

https://github.com/montagejs/matte/blob/master/test/popup/popup-spec.js#L112-L125

@cesine cesine closed this Feb 10, 2014
@francoisfrisch
Copy link
Contributor

It is supposed to be used statically, we didn't use to make the difference between constructor and prototype properties hence the problem in the spec.

@cesine
Copy link
Contributor Author

cesine commented Feb 13, 2014

ah okay, should i maybe add a test to show the static use to round off the PR?

@kriskowal
Copy link
Member

/poke @francoisfrisch

@cesine
Copy link
Contributor Author

cesine commented Jun 1, 2014

@kriskowal i dont mind adding the specs if you want to use this ;)

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.

3 participants