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

Waves on aurelia-cli project #238

Closed
wants to merge 3 commits into from

Conversation

Ullfis
Copy link
Contributor

@Ullfis Ullfis commented Aug 27, 2016

Checking if the bridge is running with global requirejs and only attach waves if not, makes it work with aurelia-cli. Also tested with webpack and jspm.

symptoms: in aurelia-cli waves is double attached and not working
as expected. This fix check if bridge is running with global requirejs.
If so, waves is already attached to elements.
@coveralls
Copy link

coveralls commented Aug 27, 2016

Coverage Status

Coverage increased (+0.02%) to 95.175% when pulling ec9652f on Ullfis:waves-amd-check into 56bea20 on aurelia-ui-toolkits:master.

@Thanood
Copy link
Collaborator

Thanood commented Aug 29, 2016

I'm still a little confused by this comment: #168 (comment)

@Ullfis
Copy link
Contributor Author

Ullfis commented Aug 29, 2016

yeah.. maybe wait until latest cli can be tested better. Worked with an earlier version.

@@ -34,7 +34,9 @@ export class MdWaves {
}

this.attributeManager.addClasses(classes);
Waves.attach(this.element);
if (!(typeof window.require === 'function' && typeof window.require.specified === 'function' && window.require.specified('waves'))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my build, window.require is not defined but instead window.requirejs.. Is it a typo or does that actually work in your build?

Also: What do you think of a build condition instead? Since it's only required with AMD modules we could simply add it to AMD build (via a gulp "remove" task).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why this works at all with elements added after DOMContentLoaded? I still don't get it (had a look at the fork's changes already).

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 am not sure why it works. Its just an observation.

window.require vs window.requirejs.. maybe it has something to do how materialize amd is build?

The remove task sound interesting 😄

Copy link
Contributor Author

@Ullfis Ullfis Aug 31, 2016

Choose a reason for hiding this comment

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

In my project both window.require and window.requirejs is defined. I will add a commit in the branch and change it to window.requirejs so it can be tested more.

i merged and did a gulp build-release in:

npm install github:ullfis/aurelia-materialize-bridge#build-branch --save

Only change from your master is the waves-amd-check branch.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.175% when pulling 094c1ae on Ullfis:waves-amd-check into 56bea20 on aurelia-ui-toolkits:master.

@Ullfis
Copy link
Contributor Author

Ullfis commented Aug 31, 2016

tested on aurelia-cli, webpack and the sample project. Works (on my computer) 🙈

@Thanood
Copy link
Collaborator

Thanood commented Aug 31, 2016

Took the AMD build condition route. 😄
Will have to test it, yet so I'm keeping this open.

@Thanood Thanood closed this Sep 5, 2016
@Ullfis Ullfis deleted the waves-amd-check branch September 5, 2016 19:21
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