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

Build: Build script breaks "Classic theme" demo #7771

Closed
jaspermdegroot opened this Issue Oct 10, 2014 · 7 comments

Comments

Projects
None yet
5 participants
@jaspermdegroot
Member

jaspermdegroot commented Oct 10, 2014

See jquery/demos.jquerymobile.com#4 (comment)

The problem is that our build script changes <link rel="stylesheet" href="../../css/structure/jquery.mobile.structure.css"> to <link rel="stylesheet" href="../css/themes/default/jquery.mobile-1.4.4.structure.min.css">.

@imaffett

This comment has been minimized.

Show comment
Hide comment
@imaffett

imaffett Oct 14, 2014

Contributor

@jaspermdegroot - I'm not too familiar with the build script, but I see in processDemos and demos.backbone it's doing the following

processedName = grunt.config.process( name + "<%= versionSuffix %>" );

I think those should just be the following. Happy to submit a PR with the fix.

processedName = name;
Contributor

imaffett commented Oct 14, 2014

@jaspermdegroot - I'm not too familiar with the build script, but I see in processDemos and demos.backbone it's doing the following

processedName = grunt.config.process( name + "<%= versionSuffix %>" );

I think those should just be the following. Happy to submit a PR with the fix.

processedName = name;
@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Oct 14, 2014

Member

@imaffett

Thanks for your offer to contribute! I am not too familiar with the build script either, but I think the problem is more this part of the script: https://github.com/jquery/jquery-mobile/blob/1.4-stable/Gruntfile.js#L147
@gseguin can you chime in here?

Member

jaspermdegroot commented Oct 14, 2014

@imaffett

Thanks for your offer to contribute! I am not too familiar with the build script either, but I think the problem is more this part of the script: https://github.com/jquery/jquery-mobile/blob/1.4-stable/Gruntfile.js#L147
@gseguin can you chime in here?

@imaffett

This comment has been minimized.

Show comment
Hide comment
@imaffett

imaffett Oct 14, 2014

Contributor

@jaspermdegroot - What about the script tag above? Should that have the version in it or not?

https://github.com/jquery/jquery-mobile/blob/1.4-stable/Gruntfile.js#L105
https://github.com/jquery/jquery-mobile/blob/1.4-stable/Gruntfile.js#L109

If that's the case, then the regex can be changed to just use name instead of processedName

Contributor

imaffett commented Oct 14, 2014

@jaspermdegroot - What about the script tag above? Should that have the version in it or not?

https://github.com/jquery/jquery-mobile/blob/1.4-stable/Gruntfile.js#L105
https://github.com/jquery/jquery-mobile/blob/1.4-stable/Gruntfile.js#L109

If that's the case, then the regex can be changed to just use name instead of processedName

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Oct 16, 2014

Contributor

@jaspermdegroot spotted it. We shouldn't be using processedName in the concatenation, but grunt.config.process( name + ".structure" + "<?= versionSuffix %>" )

Contributor

gabrielschulhof commented Oct 16, 2014

@jaspermdegroot spotted it. We shouldn't be using processedName in the concatenation, but grunt.config.process( name + ".structure" + "<?= versionSuffix %>" )

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Oct 16, 2014

Contributor

@arschmitz, @gseguin should we maybe add a travis job that runs casper on a versioned copy of dist/. I mean, like, running casper on the demos produced by grunt release:init dist?

Contributor

gabrielschulhof commented Oct 16, 2014

@arschmitz, @gseguin should we maybe add a travis job that runs casper on a versioned copy of dist/. I mean, like, running casper on the demos produced by grunt release:init dist?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Oct 16, 2014

Member

@gabrielschulhof hmm the only thing about that is it takes quite a while to run the casper tests. I will be updating the casper tests shortly to use grunt-spider which allows much better configuration for multiple test runs we can figure something out for testing this then as well. But lets not let that hold up fixing this.

Member

arschmitz commented Oct 16, 2014

@gabrielschulhof hmm the only thing about that is it takes quite a while to run the casper tests. I will be updating the casper tests shortly to use grunt-spider which allows much better configuration for multiple test runs we can figure something out for testing this then as well. But lets not let that hold up fixing this.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Oct 16, 2014

Contributor

@arschmitz Yeah, that's cool. In fact, we should separate out the casper tests. Running them with every job is kinda superfluous, unless we key in the jQuery version into the casper tests as well.

Contributor

gabrielschulhof commented Oct 16, 2014

@arschmitz Yeah, that's cool. In fact, we should separate out the casper tests. Running them with every job is kinda superfluous, unless we key in the jQuery version into the casper tests as well.

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