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

Feature/grunt bower #162

Merged
merged 28 commits into from Feb 10, 2014
Merged

Feature/grunt bower #162

merged 28 commits into from Feb 10, 2014

Conversation

johnyb
Copy link
Contributor

@johnyb johnyb commented Feb 8, 2014

We decided to use this weekend to do a little sprint to push mailvelope development a little further. We wanted to integrate a decent build workflow using grunt and add dependency management for ui libraries using bower.

While we were at it, we not only ported most Makefile targets, but also introduce new ones, like jshint. We also changed the directory structure a little. No code is generated into any subdirectory of source files any longer, but we build into the build/ directory instead. This caused some confusion so we decided it would be better to clean that up.

We did no behavioural changes, so everything should work as before. The only reason to touch some source files were to fix jshint errors. (Basically white-space issues and missing semi-colons.)

TODO (still some issues):

  • switch back to original versions (we wanted to update, but we will do that in a separate PR)
    • I force-pushed, so the versions appear to be the correct ones, from the beginning
  • switch to bootstrap from bower
  • switch to modernizr from bower (or may be use grunt-modernizr to create a custom build of the lib)
  • switch to wysihtml5 from bower
    • this has been shifted to another PR, since it seems to much work, for now
  • switch to spectrum from bower
  • switch to kendo-ui from bower
  • fix closing frame (seems broken)
  • fix Blocked script execution in 'about:blank' because the document's frame is sandboxed and the 'allow-scripts' permission is not set. when closing error message - check common/dep/jquery.ext.js
  • update development howto in readme file (npm install, grunt tasks, …)

@johnyb
Copy link
Contributor Author

johnyb commented Feb 9, 2014

Note: this would make #108 (and even #140 in a way) obsolete and also opens the possibility to introduce tests and will make translation of all the strings more easy (because there are grunt tasks available to solve some related issues)

dotcore and others added 28 commits February 9, 2014 16:57
also add first grunt modules, that will be used, later.
build-cs task from Makefile ported to grunt
copy all common files into browser dependant directories
this is used to create the chrome plugin
more easy to remember (and compatible to Makefile)
use grunt mozilla-addon-sdk to generate an xpi file. Also add an
compatibility task 'dist-ff' like in the Makefile.
build everything into a build directory and leave all source directories
untouched. This makes the clean task much more easy and simplifies
working with the code (e.g. no more opening of files in chrome/common
directory)
adjust documentation to grunt workflow
add information about clean and watch tasks
this only happens with newer jQuery, but this should be cleaned up, anyway.
don’t ask anything during `bower install`
@johnyb
Copy link
Contributor Author

johnyb commented Feb 9, 2014

Okay … this is the version, we’d like to be merged. All the open tasks have been finished and it seems to work fine for us.

@toberndo
Copy link
Member

Amazing stuff! That was definitely a productive weekend :-)
Will miss make copy-common (not really!), grunt watch is bliss.
For wysihmtl5 we maybe can't use bower as I did some modifications:
https://github.com/toberndo/mailvelope/commits/master/common/dep/wysihtml5/js/wysihtml5-0.4.0pre.js
Probably more questions to come from my side, but merging this in now.
Thanks!

toberndo pushed a commit that referenced this pull request Feb 10, 2014
@toberndo toberndo merged commit cd30dd4 into mailvelope:master Feb 10, 2014
@johnyb johnyb deleted the feature/grunt-bower branch February 10, 2014 11:03
@johnyb
Copy link
Contributor Author

johnyb commented Feb 10, 2014

That’s why we left out wysihtml5. It is possible, but would require some more work. We also experimented with bootstrap3 and this would require some work in this area anyway. So we just left it out, for now.

@toberndo
Copy link
Member

On bower install:

bower bootstrap             resolution Removed unnecessary bootstrap#~2.2.2 resolution

modifies:

   "resolutions": {
-    "jquery": "~2.0.3",
-    "bootstrap": "~2.2.2"
+    "jquery": "~2.0.3"
   }

Fixed with: da2e52a

@toberndo
Copy link
Member

fix Blocked script execution in 'about:blank' because the document's frame is sandboxed and the
'allow-scripts' permission is not set. when closing error message - check common/dep/jquery.ext.js

@johnyb Can you point to exactly where this happened? Thanks

@johnyb
Copy link
Contributor Author

johnyb commented Feb 11, 2014

On Tuesday 11 February 2014 03:54:46 Thomas Oberndörfer wrote:

fix Blocked script execution in 'about:blank' because the document's frame
is sandboxed and the 'allow-scripts' permission is not set. when closing
error message - check common/dep/jquery.ext.js
@johnyb Can you point to exactly where this happened? Thanks

Is this still happening? It used to happen when closing on of the embedded
frames. Especially if there has been an error-message in it. So there should
be a frame around an encrypted message and the message should not be
decryptable (because the key is not in the keyring). Then there will be an
error shown and if you close this frame, this error has been logged to the
console. However, I am not able to reproduce this any longer on the master
branch.

@toberndo
Copy link
Member

Is this still happening?

Could not reproduce it. Maybe happened only with newer version of jQuery?

Just wanted to know how the fix for this issue looked like. If there is no fix for a non-reproduceable problem, then ok :)

@johnyb
Copy link
Contributor Author

johnyb commented Feb 11, 2014

Could not reproduce it. Maybe happened only with newer version of jQuery?

indeed. At first, we wanted to upgrade some dependencies, but had trouble with this. So we decided to do this as a separate step. This PR should not have changed any code functionality-wise. We only changed some minor things to keep the code nanny silent.

BTW: what I haven’t mentioned, yet. We switched off a lot of checks for jshint. So the rule-set now is not very strict. May be, you want to enforce some more strict rules. But these would require larger changes to fix the current code-base.

@toberndo
Copy link
Member

BTW: what I haven’t mentioned, yet. We switched off a lot of checks for jshint. So the rule-set now
is not very strict. May be, you want to enforce some more strict rules. But these would require
larger changes to fix the current code-base.

Think it would be good to make the code compliant to use strict.

My personal preference, ignoring Crockford is:

  • function() is ok, no mandatory whitespace for anonymous functions. Hope we can deactivate this check
  • for (var i = 0; is ok, no need to declare variables at beginning of function
  • declare one variable per var statement

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.

None yet

3 participants