Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

idea: support multiple file upload #6

Closed
mozfreddyb opened this issue Feb 28, 2014 · 6 comments
Closed

idea: support multiple file upload #6

mozfreddyb opened this issue Feb 28, 2014 · 6 comments

Comments

@mozfreddyb
Copy link

It would be nice if one could upload multiple JS files at the same time.
This requires multiple codeMirror instances per file (or changing the content on the fly), some additional file navigation. Question is, would the "Output" part belong to a specific file tab or continue showing for all files?

(This surely wouldn't look nice if #4 isn't fixed though.)

@mozfreddyb
Copy link
Author

Luckily, this issue wouldn't need any rework on the scanning/upload handling. Currently, we would already scan multiple files, but a second file just overwrites previous results & editor content

@pauljt
Copy link

pauljt commented Mar 9, 2014

Done!

@pwnetrationguru
Copy link

So looks like this landed in master and ended up breaking a few things; another reason we need these tests!!! :) Also, there are conflicts with the previously existing PR #31.

In master, the client interface's CodeMirror instance is broken. It does not load our examples and I cannot manual input code into the code mirror.

Upon hitting scan, I get the following error:

Error: $scope.inputFiles[0] is undefined ScanCtrl/$scope.run@http://people.mozilla.org/~rfletcher/scanjs/client/js/scanctrl.js:11 Za.prototype.functionCall/<@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:164 Pc[c]</<.compile/</</<@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:181 Cd/this.$get</h.prototype.$eval@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:103 Cd/this.$get</h.prototype.$apply@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:104 Pc[c]</<.compile/</<@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:181 o.event.dispatch@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/jquery-2.1.0.min.js:3 o.event.add/r.handle@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/jquery-2.1.0.min.js:3


...{return typeof o!==U&&o.event.triggered!==b.type?o.event.dispatch.apply(a,argume...

We should probably back these changes out, fix the errors, and then submit a PR. jmo though, I definitely don't want to be bullheaded so please let me know your opinions @mozfreddyb and @pauljt on how best to handle.

In my opinion, in the future we should follow this process before merging into master:

  1. Tests pass - once we have them :)
  2. push dev branch and create PR describing the changes, etc.
  3. Get at least 1 LGTM from someone on the project
  4. If no conflicts, merge into master
  5. If conflicts, resolve conflicts, run tests
  6. Assuming no major changes, merge into master, else update PR.

Again, not trying to lead this development process in any one direction, just trying to figure out what we all agree on. :)

@pauljt
Copy link

pauljt commented Mar 10, 2014

It's not broken, its a feature! :) You have to load a file first now. I'll add some checks.

@pauljt pauljt reopened this Mar 10, 2014
@mozfreddyb
Copy link
Author

well if it still breaks things, we want to back out :|

@mozfreddyb
Copy link
Author

I think this was fixed with commit 58cf6cb. Close this, @pauljt?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants