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

Migrate to webpack #353

Closed
wants to merge 16 commits into from

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented May 10, 2019

Preliminary commit. Bundled files in the base template

@@ -0,0 +1,216 @@
const $ = require('jquery');
const instantsearch = require('instantsearch.js').default;

This comment has been minimized.

Copy link
@agathver

agathver May 10, 2019

Collaborator

Doesn't ES6 import work? import instantsearch from 'instantsearch.js' ?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig May 10, 2019

Author Collaborator

Works, works!

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig May 10, 2019

Author Collaborator

@agathver How would we want to enable webpack to watch our files in production?

This comment has been minimized.

Copy link
@agathver

agathver May 12, 2019

Collaborator

In production we just run a yarn build you need to modify ansible configurations to make sure it is triggered. It is similar to the cache clear task we perform. The yarn build must come before generating the Symfony cache.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig May 12, 2019

Author Collaborator

@aquibbaig Where are you running yarn dev-server inside or outside the VM?

yarn encore dev inside the vm

@aquibbaig

This comment was marked as outdated.

Copy link
Collaborator Author

aquibbaig commented May 10, 2019

TODO

  • Style to the Algolia search-input
  • Add more page dependent js to webpack
  • Minimise global variables as much as possible
  • Accessing classification hierarchy data inside project-settings
  • Toggle projects, organizations on click in the search page
@agathver

This comment has been minimized.

Copy link
Collaborator

agathver commented May 12, 2019

@aquibbaig Where are you running yarn dev-server inside or outside the VM?

@aquibbaig aquibbaig referenced this pull request May 12, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:encore branch 2 times, most recently from 50dcd8c to 6bf68f4 May 17, 2019
@aquibbaig aquibbaig referenced this pull request May 19, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:encore branch from 6bf68f4 to 81f2c60 May 21, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented May 24, 2019

TODO

  • Style to the Algolia search-input
  • Add more page dependent js to webpack
  • Minimise global variables as much as possible
  • Accessing classification hierarchy data inside project-settings
  • Toggle projects, organizations on click in the search page
  • Remove Assetic completely
  • Set up production build

updates

@aquibbaig aquibbaig force-pushed the aquibbaig:encore branch 2 times, most recently from 56b1a86 to 451d977 May 25, 2019
Copy link
Contributor

imphil left a comment

Thanks for your work on this @aquibbaig! I've left comments in the various places of the code, but we're going into the right direction!

Please also don't forget to tell me which yarn commands need to be run as part of the deployment so that I can add them to the Ansible scripts.

become: true
apt_key:
url: "{{item}}"
with_items:
- 'https://github.com/rabbitmq/signing-keys/releases/download/2.0/rabbitmq-release-signing-key.asc'
- 'https://packagecloud.io/rabbitmq/rabbitmq-server/gpgkey'
- 'https://packages.erlang-solutions.com/ubuntu/erlang_solutions.asc'

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

Please revert those changes as discussed. If bintray repositories don't work for you we need to find another solution in another PR.

@@ -1,24 +0,0 @@
---

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

This file needs to stay.

@@ -19,3 +19,5 @@
!bin/console
!bin/symfony_requirements

/node_modules/*
/web/build/*

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

Please add a trailing newline.

<script src="{{ asset_url }}"></script>
{% endjavascripts %}
<script src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
<script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
<![endif]-->

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

Please remove the <IE9 compat code as discussed.

{% endjavascripts %}

<script type="text/javascript">
<script type="text/javascript">

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

nit: indentation

* Each entry will result in one JavaScript file (e.g. app.js)
* and one CSS file (e.g. app.css) if you JavaScript imports CSS.
*/
//css entries

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

always in this file: // CSS entries (space after // and comment)

.addStyleEntry('app-css', ['./web/assets/scss/librecores.scss',
'./web/assets/scss/bootstrap-librecores.scss',
'./web/assets/css/bootstrap-social.css',
'./web/assets/css/font-awesome.min.css',

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

Can you install bootstrap-social and fontawesome through yarn?

'./web/assets/css/font-awesome.min.css',
'./web/assets/css/librecores.css'])
.addStyleEntry('trumbowyg', ['./web/assets/scss/trumbowyg.scss'])
.addStyleEntry('project_new_css','./web/assets/css/bootstrap-select.css')

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor
  • space after the ,
  • for consistency, always give the second argument as array
.addEntry('project_new', ['./web/assets/js/bootstrap-select.js',
'./web/assets/js/project_new.js'])
.addEntry('planet', './web/assets/js/planet.js')
.addEntry('organisation', './web/assets/js/organisation.js')

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

Please make these file names consistently using the page name.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig May 28, 2019

Author Collaborator

Knew that naming conventions would be a problem. What do you recommend?
Planet - planet_page , Organisation - organisation_page?

This comment has been minimized.

Copy link
@imphil

imphil May 28, 2019

Contributor

Ideally you find a convention that matches the URL fragment and/or the html template (in views). So most of them are fine most likely, except for project-view.

On the CSS side, project_new_css and trumbowyg have inconsistent names.

And please think about app-js and app-css. Do you need the js/css suffixes? And can we keep things consistent by either using underscores or dashes?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig May 30, 2019

Author Collaborator

Sure thing, We can use base.css base.js as they are in base.html.twig

'./web/assets/scss/bootstrap-librecores.scss',
'./web/assets/css/bootstrap-social.css',
'./web/assets/css/font-awesome.min.css',
'./web/assets/css/librecores.css'])

This comment has been minimized.

Copy link
@imphil

imphil May 27, 2019

Contributor

Format this in a way that makes it easy to see the list of file. And add a trailing comma to make the list easy to extend without merge conflicts or ugly diffs.

.addStyleEntry('app-css', [
  './web/assets/scss/librecores.scss',
  './web/assets/scss/bootstrap-librecores.scss',
  './web/assets/css/bootstrap-social.css',
  './web/assets/css/font-awesome.min.css',
])

(Equally apply this to the other code in this file.)

@aquibbaig aquibbaig force-pushed the aquibbaig:encore branch from 91e7e2e to 1566860 May 29, 2019
}

if(days > 0) {
return days === 1 ? days + ' day ago' : days + ' day ago';

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig May 30, 2019

Author Collaborator

Here I guess, okay!

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented May 31, 2019

@aquibbaig please let me know when you're done with your changes and when I should take another look.

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented May 31, 2019

@imphil yeah, sure. Had to configure a loader into webpack for parsing classifications.yml from the Resources directory, which needs some object manipulation to be sent inside the function. Will see and push things tomorrow after my exam

Copy link
Contributor

imphil left a comment

There are still a couple of unresolved comments. Please let me know when you feel all comments have been resolved.

"moment": "^2.24.0",
"node-sass": "^4.12.0",
"popper.js": "^1.15.0",
"sass-loader": "^7.1.0"

This comment has been minimized.

Copy link
@imphil

imphil Jun 2, 2019

Contributor

Yes, everything here should most likely not be a devDependency, but a normal dependency.

@@ -1,5 +1,12 @@
// Livestamp.js / v2.0.0 / (c) 2015 Matt Bradley / MIT License

This comment has been minimized.

Copy link
@imphil

imphil Jun 2, 2019

Contributor

How does that answer the question I raised? Why can't we install livestamp through yarn?

@@ -26,7 +26,6 @@
Package: erlang*
Pin: release o=Bintray
Pin-Priority: 1000

This comment has been minimized.

Copy link
@imphil

imphil Jun 2, 2019

Contributor

Please remove this change as well.

@aquibbaig aquibbaig referenced this pull request Jun 3, 2019
import $ from 'jquery';
import 'bootstrap';
// uncomment to support legacy code
global.$ = global.jQuery = $;

This comment has been minimized.

Copy link
@imphil

imphil Jun 3, 2019

Contributor

Missing a trailing newline. Please configure your editor to always output trailing newlines.

@aquibbaig aquibbaig marked this pull request as ready for review Jun 3, 2019
@aquibbaig aquibbaig requested review from imphil and agathver Jun 3, 2019
@agathver agathver added the gsoc label Jun 3, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jun 3, 2019

  • Can you install bootstrap-select.js and bootstrap-markdown.js through yarn?
    • If not, at least remove the executable bit from bootstrap-select.js
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jun 4, 2019

  • Can you install bootstrap-select.js and bootstrap-markdown.js through yarn?

    • If not, at least remove the executable bit from bootstrap-select.js

Sure! One thing: I didn't find where we actually use bootstrap-markdown. For now it's added to base.js to be available across all the templates. Do tell me where to move it.

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jun 7, 2019

Thanks! This now needs me updating the Ansible files and testing, won't get to that before the weekend.

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jun 9, 2019

This required some more changes, but it's merged now through 2dfec83

Looks like you were following an old tutorial, some things are much easier if you follow the most recent documentation. There's now only one addEntry call per site, and the CSS is included through the JS. All JS/CSS link/script tags are generated by encore_* twig functions, which makes it much more robust.

You also moved to bootstrap 4, which wasn't fully working in various places. I stayed with bootstrap 3 and opened #380 to track the move to a newer version.

I also made sure to remove all bundled JS/CSS files.

@imphil imphil closed this Jun 9, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jun 9, 2019

@imphil The changes make a lot more sense. Thanks

@aquibbaig aquibbaig referenced this pull request Jun 14, 2019
@aquibbaig aquibbaig added gsoc 2019 gsoc and removed gsoc labels Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.