search

gitcoinco / skunkworks

experimental laboratory
63 stars 35 forks source link

Initial commit to Upload refactor #48

Closed eswarasai closed 6 years ago

eswarasai commented 6 years ago

Issue #22

eswarasai commented 6 years ago

@owocki -- I've few questions related to the task. Please find them below :

1) Currently, in Backend we only accept images of type PNG/JPEG. Let me know if we need to add provision to allow the GIF as well. 2) X MB is the max size the script can handle -- What should be the max size of the uploaded image? 3) If User tries to upload more than 10 images, currently I'm not displaying any error message. Instead I'm just picking the first 10 images and displaying them. Let me know if this behaviour needs to be changed. 4) I've handled the migrations but left the legacy data as it is in the database without removing the columns. Not sure if this is required at the moment?

I've one suggestion w.r.t. UI. Instead of having the preview images side by side, I think a slider might be much better with the count on top or something like that.

Let me know your thoughts/suggestions on the task. Thanks.

owocki commented 6 years ago
  1. if its easy to do, itd be nice
  2. defer to @stojce -- my emperical testing has shown that 2-3 MBs are safe... and i have had a few images at 10MB that have not worked
  3. it'd be nice to just not let them add more than 10 at once... does that work?
  4. it'd be nice to have the legacy data at least have an updated description field for SEO purposes.. this shouldnt be hard to do..

I've one suggestion w.r.t. UI. Instead of having the preview images side by side, I think a slider might be much better with the count on top or something like that.

would love to see a screenshot.. but at initial glance im ok with idea

owocki commented 6 years ago

have you tested this PR end to end?

eswarasai commented 6 years ago

@owocki -- Yes, I've tested this PR right from uploading images on the Frontend till the data being saved appropriately in the Backend along with the DB migrations. I haven't tested the part of approving the images for them to appear on the Dashboard though.

1) Should be easy to add support for GIF. I'll give it a try and update you 2) Sure. The Dropzone plugin provides an option to limit the size of the images. I'll try adding a limit of 5 MB to be on safe side 3) The Dropzone plugin doesn't provide a default limit to number of files that can be selected in the system file picker. This needs to be handled programatically after the User selects the images in the system file picker. So either we can throw an error message and disallow the import or we can stick to the current implementation of selecting first 10 images 4) The migration currently populates just the Title field to the Description like requested earlier. Let me know if there's a format text you want me to use in the migration with the combination of the fields Title, Author, Author Email and Tags

Regarding the UI changes for the slider, you can take a glance here at the section Thumbs Gallery With Two-way Control -- http://kidjp85.github.io/react-id-swiper/

owocki commented 6 years ago

nice.. looking forward to playing with this! let me know when its' ready for review

owocki commented 6 years ago

hey @eswarasai do you think you can get this across the finish line in the next couple days?

eswarasai commented 6 years ago

Yes @owocki. I'm currently working on implementing the Swiper Gallery for the Images. Should be done within a day or two. Thanks.

eswarasai commented 6 years ago

@owocki -- I've committed the changes for the above items. Let me know if there's anything else needed. Thanks.

owocki commented 6 years ago

great... is it easy to deploy somewhere online for testing?

@stojce if i just git pull once this is in master, on the production server, is that prettymuch the deployment process?

eswarasai commented 6 years ago

@owocki -- I'm not sure about the online deployment. Maybe @stojce can help us out here.

stojce commented 6 years ago

@owocki @eswarasai Yes, you should merge this PR and thengit pull from server. Let me know if you have any issues.

eswarasai commented 6 years ago

@stojce -- Before merging it to master and releasing it, @owocki would like to test it online. Can you let us know with the deployment steps? TIA.

owocki commented 6 years ago

stojce just commented with the deployment steps...

we don't have a staging environment to my knowledge.

stojce commented 6 years ago

Maybe pull to another folder on the server and create sub domain. It would be mislev to have ‘develop’ branch in that case and pull from that one. Also, if you run ‘migrate’ live DB would be affected.

eswarasai commented 6 years ago

@owocki -- I was referring to getting ethwallpaper getting deployed on a different server like staging and not on production.

@stojce -- Let me know if we can setup on Heroku or AWS Free instance along with the instructions. Thanks.

stojce commented 6 years ago

@eswarsai I cannot decide that. Please talk to @owocki

owocki commented 6 years ago

@owocki -- I was referring to getting ethwallpaper getting deployed on a different server like staging and not on production.

yes im aware.

im on calls all day today. if you can find a way to deploy to heroku or some other provider on your own, thats the preference from my side.

eswarasai commented 6 years ago

@owocki -- Sure. I'll give it a try and see if I can do the deployment