Skip to content

Use Webpack Encore to manage assets #586

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.

Sign up for GitHub

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

Closed
javiereguiluz wants to merge 4 commits into from
Closed

Use Webpack Encore to manage assets #586

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fbc840
Use Webpack Encore to manage assets
javiereguiluz Jun 12, 2017
64998f8
Added the compiled assets to make the app work out of the box
javiereguiluz Jun 12, 2017
0fbf7cc
More work on assets
javiereguiluz Jun 12, 2017
fd4e4d8
More work on assets
javiereguiluz Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/app/config/parameters.yml
/build/
!/web/build/
Copy link
Member

@stof stof Jun 13, 2017

Choose a reason for hiding this comment

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

what is this rule about ? AFAIK, it does not do anything, as the folder is not excluded before (/build/ does not match it AFAIK, unlike build/ would do)

/node_modules/
/phpunit.xml
/.php_cs
/var/*
Expand All @@ -16,3 +18,5 @@
!var/SymfonyRequirements.php
/vendor/
/web/bundles/
/web/build/fonts/glyphicons-*
/web/build/images/glyphicons-*
69 changes: 69 additions & 0 deletions app/Resources/assets/js/admin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
var $ = require('jquery');
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

What about putting the assets directory at the root of the project? It's a bit more "flex" like

Copy link
Member Author

@javiereguiluz javiereguiluz Jun 13, 2017

Choose a reason for hiding this comment

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

Yes ... but let's do that when we switch the entire app to Flex.

window.$ = $;
window.jQuery = $;
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

You shouldn't need either of these unless we still have some JavaScript code in a template somewhere that is relying on these to be globally available. The only place I could see was login.html.twig. We could move that little bit of JavaScript here, or into a new login entry as an example of a page-specific JS file.

Copy link
Member Author

@javiereguiluz javiereguiluz Jun 13, 2017

Choose a reason for hiding this comment

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

If I remove it, I see this error in any backend page:

admin.js:38009 Uncaught TypeError: Cannot read property 'fn' of undefined
    at admin.js:38009
    at Object.<anonymous> (admin.js:38134)
    at __webpack_require__ (admin.js:20)
    at Object.<anonymous> (admin.js:35021)
    at __webpack_require__ (admin.js:20)
    at Object.<anonymous> (admin.js:42417)
    at __webpack_require__ (admin.js:20)
    at module.exports.ctor.super_ (admin.js:66)
    at admin.js:69


require('bootstrap-sass');
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

How about a comment above this:

// loads the Bootstrap jQuery plugins

require('moment');
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

This shouldn't be needed. I think only the eonasdan-bootstrap-datetimepicker library uses it, and it should (and appears to) require moment internally on its own https://github.com/Eonasdan/bootstrap-datetimepicker/blob/25c11d79e614bc6463a87c3dd9cbf8280422e006/src/js/bootstrap-datetimepicker.js#L42

require('eonasdan-bootstrap-datetimepicker');

require('imports-loader?define=>false!typeahead.js/dist/typeahead.jquery.min.js');
Copy link
Member

@stof stof Jun 13, 2017

Choose a reason for hiding this comment

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

why using the imports-loader ? The dist files are using UMD, so they should work fine with CommonJS requirements.

I think all your issues about defining global variables are actually related to the weird loading you do here.

Copy link
Member Author

@javiereguiluz javiereguiluz Jun 13, 2017

Choose a reason for hiding this comment

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

I have no idea ... I spent a lot of time trying to make typeahead/bloodhound work. Nothing worked. This was one of the tests that I did after reading some of the issues reported about this library (there are like a lot of issues reported!)

const Bloodhound = require('imports-loader?define=>false!typeahead.js/dist/bloodhound.js');
window.Bloodhound = Bloodhound;
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

Again, this shouldn't be needed (in theory), but it's possible there's some legacy library that isn't requiring this correctly. We'll need to make sure each line is needed, and document which lines are needed and why

Copy link
Member Author

@javiereguiluz javiereguiluz Jun 13, 2017

Choose a reason for hiding this comment

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

If I remove those, I see this error:

Uncaught ReferenceError: Bloodhound is not defined
    at HTMLDocument.<anonymous> (admin.js:35043)
    at mightThrow (admin.js:11908)
    at process (admin.js:11976)

Copy link
Member Author

@javiereguiluz javiereguiluz Jun 13, 2017

Choose a reason for hiding this comment

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

I see a lot of errors related to this library. Maybe we can replace it with some simpler alternative library? After all, our needs are super basic.

Copy link
Contributor

@alOneh alOneh Jun 13, 2017

Choose a reason for hiding this comment

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

add something like .autoProvideVariables({ 'window.Bloodhound': "bloodhound") to your webpack.config.js

Copy link
Member

@stof stof Jun 13, 2017

Choose a reason for hiding this comment

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

We might decide to change: twitter/typeahead.js#1618

require('bootstrap-tagsinput');

$(document).ready(function () {
Copy link
Member

@stof stof Jun 13, 2017

Choose a reason for hiding this comment

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

this should be changed to just $(function() {...}) IMO, which is the proper jQuery API (no need to create a jQuery collection for the document, while .ready() does not care about the collection)

// Datetime picker initialization.
// See http://eonasdan.github.io/bootstrap-datetimepicker/
$('[data-toggle="datetimepicker"]').datetimepicker({
icons: {
time: 'fa fa-clock-o',
date: 'fa fa-calendar',
up: 'fa fa-chevron-up',
down: 'fa fa-chevron-down',
previous: 'fa fa-chevron-left',
next: 'fa fa-chevron-right',
today: 'fa fa-check-circle-o',
clear: 'fa fa-trash',
close: 'fa fa-remove'
}
});

// Bootstrap-tagsinput initialization
// http://bootstrap-tagsinput.github.io/bootstrap-tagsinput/examples/
var $input = $('input[data-toggle="tagsinput"]');
if ($input.length) {
var source = new Bloodhound({
local: $input.data('tags'),
queryTokenizer: Bloodhound.tokenizers.whitespace,
datumTokenizer: Bloodhound.tokenizers.whitespace
});
$input.tagsinput({
trimValue: true,
focusClass: 'focus',
typeahead: {
name: 'tags',
source: source
}
});
}
});

// Handling the modal confirmation message.
$(document).on('submit', 'form[data-confirmation]', function (event) {
var $form = $(this),
$confirm = $('#confirmationModal');

if ($confirm.data('result') !== 'yes') {
//cancel submit event
event.preventDefault();

$confirm
.off('click', '#btnYes')
.on('click', '#btnYes', function () {
$confirm.data('result', 'yes');
$form.find('input[type="submit"]').attr('disabled', 'disabled');
$form.submit();
})
.modal('show');
}
});
13 changes: 13 additions & 0 deletions app/Resources/assets/js/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var $ = require('jquery');
window.$ = $;
window.jQuery = $;
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

Should be able to remove lines 2 and 3

Copy link
Member Author

@javiereguiluz javiereguiluz Jun 13, 2017

Choose a reason for hiding this comment

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

If I remove them, I see this error:

Uncaught TypeError: $input.tagsinput is not a function
    at HTMLDocument.<anonymous> (admin.js:35050)
    at mightThrow (admin.js:11908)
    at process (admin.js:11976)

Copy link
Member

@stof stof Jun 13, 2017

Choose a reason for hiding this comment

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

then it means that the typeahead plugin does not support getting jQuery through CommonJS


require('bootstrap-sass/assets/javascripts/bootstrap/dropdown.js');
require('bootstrap-sass/assets/javascripts/bootstrap/modal.js');
require('bootstrap-sass/assets/javascripts/bootstrap/transition.js');

var hljs = require('highlight.js');
hljs.registerLanguage('php', require('highlight.js/lib/languages/php'));
hljs.registerLanguage('twig', require('highlight.js/lib/languages/twig'));
window.hljs = hljs;
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

This shouldn't be needed

hljs.initHighlightingOnLoad();
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

does this need to be in a document.ready?

26 changes: 26 additions & 0 deletions app/Resources/assets/scss/admin.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
@import "~bootswatch/flatly/variables";
@import "~eonasdan-bootstrap-datetimepicker/src/sass/bootstrap-datetimepicker-build.scss";
@import "~bootstrap-tagsinput/src/bootstrap-tagsinput.css";
Copy link
Contributor

@weaverryan weaverryan Jun 13, 2017

Choose a reason for hiding this comment

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

When a CSS file is a dependency of some JS library, I usually like to require the CSS file right in our .js file, right next to where I require the related JS file. Later, if we stopped needing the bootstrap-datetimepicker JS file, we would also remove the CSS require right next to it.


/* Page: 'Backend post index'
------------------------------------------------------------------------- */
body#admin_post_index .item-actions {
white-space: nowrap
}

body#admin_post_index .item-actions a.btn + a.btn {
margin-left: 4px
}

/* Page: 'Backend post show'
------------------------------------------------------------------------- */
body#admin_post_show .post-tags .label-default {
background-color: #e9ecec;
color: #6D8283;
font-size: 16px;
margin-right: 10px;
padding: .4em 1em .5em;
}
body#admin_post_show .post-tags .label-default i {
color: #95A6A7;
}
Loading