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

Fix invalid ng-file-upload paths #724

Closed
wants to merge 1 commit into from

Conversation

simison
Copy link
Member

@simison simison commented Jul 28, 2015

Did a fresh clone and tested 0.4.0 branch; found out there was an invalid path for ng-file-upload module.

It's actually very out-dated (1.1.5 and newest is 5.0.9) but I don't have time now to test if that would work.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@simison I submitted a PR for this fix #722. Wasn't aware that src/module.js was the correct asset for the default config. I, probably, mistakenly used the .min.js file in dist for default.

@simison
Copy link
Member Author

simison commented Jul 28, 2015

Ah, cool. I'm traveling so following mean partially only.

I'd ditch the whole .min.js set from prod conf but that's another discussion.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@simison Thank you. So using the src/module.js is definitely the correct choice?

@simison
Copy link
Member Author

simison commented Jul 28, 2015

For now it seems to be. There's some other bug disturbing profile editing now so I can't really test if the file upload itself works now with it — it throws some mongo error.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

Good point. We're working on getting the PR merged for the profile edit bug #713. I can test the actual file upload, because I have the proposed fixes in a local branch.

EDIT: I believe I have verified the upload works. I was testing the edit profile fixes. But I'll verify that I was using ng-file-upload angular-file-upload 1.1.8.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

LGTM. I've confirmed, the profile picture upload works with this fix.

@simison
Copy link
Member Author

simison commented Jul 28, 2015

👍

@simison
Copy link
Member Author

simison commented Jul 28, 2015

BTW if you have working setup and time, I'd also look into updating ng-file-upload plugin a few versions up. Would be pretty straightforward to test.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@trainerbill thought we should look into 1.2. I'll look into the more recent versions you mentioned.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@simison I think you were getting the file upload plugins mixed up. We're using angular-file-upload https://github.com/nervgh/angular-file-upload/releases

This fix is still accurate.

@simison
Copy link
Member Author

simison commented Jul 28, 2015

Oh. Confusing. :-) Thanks.

@ilanbiala
Copy link
Member

@mleanos @trainerbill is 1.2.0 also using the same structure?

@trainerbill
Copy link
Contributor

I'm not sure... I just thought since we had to look into it we may as well make it work with the most recent version... I'll let @mleanos decided whats best.

@lirantal lirantal added this to the 0.4.0 milestone Jul 28, 2015
@ilanbiala
Copy link
Member

@mleanos let me know if it makes sense to move to 1.2.0.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@ilanbiala Actually, it doesn't. Instead of src/module.js in 1.1.8, the file looks to be src/index.js in 1.2.. I'm about to test 1.2 out. I'll report back shortly.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

Changing config/assets/default to use the src/index.js from 1.2 doesn't work; the index.js file doesn't seem to be valid, and there isn't any other file (that I see) that can be used for the default asset. I'm getting the injection error. Looks like they have some issues to work out with that asset file.

Without really digging deep into why the 1.2 src/index.js file doesn't work, I'd recommend that we stick with 1.1.8 for now.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@ilanbiala I have confirmed that 1.2 does work when using the .min file for the default asset config; and the upload functionality works as well. I know that @simison didn't seem to think it was a good idea to use the .min file.

@trainerbill
Copy link
Contributor

@mleanos Why don't we just wait till 1.2 is functioning properly and add=1.1.5 to bower.json and be done with this in 0.4.0?

@mleanos
Copy link
Member

mleanos commented Jul 28, 2015

@trainerbill Fine by me. I don't see a problem with that. @ilanbiala @simison ?

@mleanos
Copy link
Member

mleanos commented Jul 29, 2015

@trainerbill I actually just updated my app by setting my bower.json to use "angular-file-upload": "1.1.5". I'm deploying to Heroku, and obviously it wants to install 1.1.8. I think this is fine to do, and wait 1.2 works; just as you suggested.

@ilanbiala
Copy link
Member

@mleanos I'd actually prefer to keep deps steady right now until we release, so can we just change this PR to change the bower.json to 1.1.5 static?

@mleanos
Copy link
Member

mleanos commented Jul 29, 2015

@ilanbiala That sounds good to me. @simison mentioned he was traveling atm. Should I submit a new PR?

@mleanos
Copy link
Member

mleanos commented Jul 29, 2015

Just added PR #729 to fix this.

ilanbiala added a commit that referenced this pull request Jul 29, 2015
Update angular-file-upload to use static dependency
Fixes #722, Fixes #724
@ilanbiala ilanbiala closed this Jul 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants