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

V2 use npm #1782

Closed
wants to merge 18 commits into from
Closed

V2 use npm #1782

wants to merge 18 commits into from

Conversation

tcitworld
Copy link
Member

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? kinda
Documentation no
Translation no
Fixed tickets #1759
License MIT

What it does :

  • Removes Assetic Bundle
  • Uses npm to fetch most of the assets libraries
  • Uses bower to fetch a few more (fonts)
  • Uses grunt to :
    • concat css and js files
    • use browserify for all scripts that call require()
    • minify css & js
    • Symlink and copy the rest of assets
  • moved our own assets to app/Ressources/static
  • moved imgs used by both themes to _global.

What doesn't work :

  • Annotator isn't detected for some reason (inline js maybe the cause)
  • Icomoon icons in Material theme don't show up (wrong path to font file ?)
  • Can't make Travis build assets on only one build and in the same time make npm launched by composer.
  • Find a way to add MathJax Support of mathjax #1408

@tcitworld
Copy link
Member Author

Well, I changed a whole lot of things here. The only problem that's left is how to install assets only on one build on Travis.

@@ -16,10 +16,10 @@ function retrievePercent(id) {
if (!supportsLocalStorage()) { return false; }

var bheight = $(document).height();
var percent = localStorage["poche.article." + id + ".percent"];
var percent = localStorage["wallabag.article." + id + ".percent"];
Copy link
Member

Choose a reason for hiding this comment

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

:trollface:

Copy link
Member Author

Choose a reason for hiding this comment

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

I got rid of nostalgia. :)

Copy link
Member

Choose a reason for hiding this comment

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

😢

"Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::prepareDeploymentTarget",
"npm install",
"bower install",
"grunt"
Copy link
Member

Choose a reason for hiding this comment

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

This won't allow us to use a separate build on travis to run asset install.
Don't you started something with a .sh file to handle that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but it didn't got me anyway. It launched an extra build for travis instead of using an existing one.

Copy link
Member

Choose a reason for hiding this comment

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

This could still be a good idea. Look at how I've managed to check for CS & for translated file in one extra build. I prefer one more build instead of all builds taking more than 7 minutes to run ..

Copy link
Member Author

Choose a reason for hiding this comment

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

Thing is, we want to install assets by default for the end-user. So the environment parameter should tell my script not to run in some cases.

@nicosomb nicosomb modified the milestones: 2.0.0, 2.1.0 Apr 3, 2016
@nicosomb nicosomb closed this Apr 18, 2016
@tcitworld tcitworld mentioned this pull request Jun 24, 2016
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants