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

Add profile image upload component #5355

Merged
merged 1 commit into from
Jul 7, 2015

Conversation

acburdine
Copy link
Member

closes #5334

  • adds component for profile images (with optional gravatar)

TODOS:

  • Fix image previewing
  • Fix uploading after signin
  • Fix gravatar bound style property warning
  • Add tests
  • Code cleanup
  • Squash commits

@acburdine
Copy link
Member Author

@ErisDS Currently the gravatar part works. There's a couple of issues I ran into with the file upload though.

First, upload requests have been returning 401 errors, resulting in the file uploader not working, meaning the uploads api needs to be refactored to allow image uploads for setup, or some other workaround needs to be determined.

Second, to trigger the file upload, there needs to be a <input type="file"> somewhere in the html, and I'm not quite sure how to do that with the current css that's there.

@ErisDS
Copy link
Member

ErisDS commented May 28, 2015

Needing the file input is something you'd likely need @JohnONolan's help with - it'll likely need to be placed where the upload icon <a> is at the moment and made transparent so it can be clicked/dragged to but not seen.

With regard to the upload failing, I think we're going to need to do some fancy handling of what-happens-when here. If we use the preview feature of blueimp to display the image, and then send the image with the setup that could work? That would need the setup API endpoint to be adjusted to accept that image I guess?

export default Ember.Component.extend({
email: '',
size: 90,
hasUploadedImage: true,

This comment was marked as abuse.

@acburdine
Copy link
Member Author

@ErisDS Couldn't the form send the setup form first (through the changes in #5359), and then send the upload image request? IMO that would be simpler and easier than trying to attach the image to the json post request. It would reduce the needed api modifications as well.

Also if this is planned to be a re-usable component within the application (which would mean the user would be logged in), it would POST to the api/uploads route anyways.

@ErisDS
Copy link
Member

ErisDS commented May 30, 2015

That sounds perfectly sensible 👍

@ErisDS
Copy link
Member

ErisDS commented Jun 8, 2015

Ditto here - is there anything else you need from me on this one?

@acburdine
Copy link
Member Author

I'm kind of waiting on #5359. I think I've figured out most of how I want to do it so once that pr is merged it shouldn't take long

@acburdine
Copy link
Member Author

@jaswilli @novaugust Some of the things I had to do for this seemed to me to be hacks, so I wouold greatly appreciate some feedback on some of the things I did to make this work 😄

return `background-image: url(${url})`.htmlSafe();
}),

imageBackground: Ember.computed('email', function () {

This comment was marked as abuse.

@acburdine acburdine changed the title [WIP] Add profile image upload component Add profile image upload component Jun 13, 2015
* @param {Boolean} hasEmail boolean value corresponding to whether the email property has been passed in
* @param {String} defaultImage String containing the background-image css property of the default user profile image
* @param {String} imageBackground String containing the background-image css property with the gravatar url
*/

This comment was marked as abuse.

@acburdine
Copy link
Member Author

Couple of random Travis fails, but I think all the bugs have been ironed out on this one 😄

@ErisDS
Copy link
Member

ErisDS commented Jun 26, 2015

@jaswilli & @novaugust any more code review pointers on this one or is it good to go from that perspective?

@acburdine
Copy link
Member Author

Can this be merged before #5399? That way I can kill the merge conflicts there instead of here. Although I guess it doesn't matter, as there will be merge conflicts either way.

@cobbspur
Copy link
Member

cobbspur commented Jul 3, 2015

@acburdine I've given this a test run. Only potential problem from a functionality point of view is that if you upload an invalid file, It will not display anything, it should probably be set back to the initial placeholder image in this case

closes TryGhost#5334
- adds component for profile images (with optional gravatar)
- integrates image profile component into setup form
ErisDS added a commit that referenced this pull request Jul 7, 2015
Add profile image upload component
@ErisDS ErisDS merged commit 9114889 into TryGhost:master Jul 7, 2015
@acburdine acburdine deleted the onboard-profile branch August 5, 2015 01:28
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.

User Onboarding - Upload Profile Image Component
5 participants