added ajaxy image uploading #1739

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

jipiboily commented Jun 9, 2012

Here is the handling of image uploading with an ajaxy touch. It will upload files one by one instead of a single batch. This will prevent timeouts on say, Heroku.

It was asked by @robyurkowski here https://github.com/resolve/refinerycms/issues/1329#issuecomment-4862916. It was previously handled by https://github.com/jipiboily/refinerycms-imajax.

There is only one problem I see is the handling of I18n on the client side. I used the same method as you use in core, but it proved to be inefficient while I tested that. All the output will be in english even if there is a french translation. How would you want to support that?

Hope it's up to what you expected, if not, do not hesitate to comment or make suggestions. Friendly bashing accepted! ;)

This pull request passes (merged 8a76e35 into 10337cb).

@parndt parndt and 1 other commented on an outdated diff Jun 11, 2012

images/app/views/refinery/admin/images/_form.html.erb
@@ -5,6 +6,24 @@
:object => @image,
:include_object_name => false %>
+ <% unless @image.errors.any? %>
@parndt

parndt Jun 11, 2012

Owner
<% if @image.errors.empty? %>
@jipiboily

jipiboily Jun 12, 2012

Contributor

Will modify

@parndt parndt and 1 other commented on an outdated diff Jun 11, 2012

images/app/views/refinery/admin/images/_form.html.erb
@@ -5,6 +6,24 @@
:object => @image,
:include_object_name => false %>
+ <% unless @image.errors.any? %>
+ <div id="upload_progress"></div>
+
+ <div class="errorExplanation" id="errorExplanation" style="display:none;">
+ <p><%= t("refinery.admin.error_messages.problems_in_following_fields")%></p>
+ <ul id="error_list">
+
+ </ul>
+ </div>
+
+ <div id="flash_container">
+ <div id="flash" class="flash flash_notice" style="width: auto; display:none;">
+ <h3><%= t(".uploaded_successfully") %>:</h3>
+
@jipiboily

jipiboily Jun 12, 2012

Contributor

Hum, copy pasting, even your own code, is bad. That was part of what I did in imajax, don't know why I did that at first, but it isn't used at all. Will remove.

@jipiboily

jipiboily Jun 12, 2012

Contributor

No, this is used, sorry. This is the final confirmation message if everything worked correctly.

http://cl.ly/1B1j0a2B0h0S1N2P3e0b

@parndt

parndt Jun 12, 2012

Owner

Ah okay - in the screenshot the spacing looks a bit out :s

@jipiboily

jipiboily Jun 13, 2012

Contributor

Not sure you would like me to fix that. Should I create a scss file in the image gem with the fix?

@parndt

parndt Jun 13, 2012

Owner

Yes please - just so that the icon lines up with the header (probably by removing the margin-top) thanks!

@jipiboily

jipiboily Jun 20, 2012

Contributor

didn't forget you @parndt. I have some time planned for open source this evening and will do that first.

@jipiboily

jipiboily Jun 20, 2012

Contributor

I finally added the style inline (9dbeccf). I could have added a new manifest file in images/app/assets/stylesheets/images.css, but it might be a bit of an overhead for a single style? If you prefer that, just say it and I'll change that in a blast (today I mean! :P). Just let me know what you prefer...

@parndt parndt and 1 other commented on an outdated diff Jun 11, 2012

images/spec/requests/refinery/admin/images_spec.rb
end
+ find(".ui-icon-closethick").click
@parndt

parndt Jun 11, 2012

Owner

should this be an anchor tag that can then be tested like:

click_link 'ui-icon-closethick'
@jipiboily

jipiboily Jun 12, 2012

Contributor

This doesn't work as it needs to be an id (or text) and it is a class. I didn't want to add an id so I used find, which can use any css selector. Maybe I missed something?

@phiggins phiggins and 1 other commented on an outdated diff Jun 11, 2012

...s/app/assets/javascript/refinery/images.js.coffee.erb
+ upload_all: ->
+ @before_upload_all()
+ for file in @file_list
+ @validate(file)
+ @files.push file
+ return @render_errors() if @errors.length > 0
+ $("#submit_button").hide()
+ $(".field").hide()
+ @progress "<%= ::I18n.t("refinery.admin.images.form.uploading")%>"
+ @upload_next()
+
+ after_upload_all: ->
+ $(".save-loader").hide()
+ @upload_progress.hide()
+ if @errors.length > 0
+ @progress "<%= ::I18n.t("refinery.admin.images.form.there_was")%> #{@errors.length} <%= ::I18n.t("refinery.admin.images.form.error")%>#{(if @errors.length > 1 then "s" else "")}"
@phiggins

phiggins Jun 11, 2012

Contributor
  1. You can interpolate in translations using a special syntax.
  2. Pluralize takes an argument that determines if it should be pluralized or not.

With both of those in mind, you could change this to something like:

::I18n.t("refinery.admin.images.form.error_count", :default => "There was %{error_count} %{error_plural).", :error_count => @errors.length, :error_plural => 'error'.pluralize(@errors.length))

Still not perfect because of the was/were discrepancy, but you get the idea.

@jipiboily

jipiboily Jun 12, 2012

Contributor

In fact, this is not used anymore. It is rendered here: https://github.com/resolve/refinerycms/pull/1739/files#L1R12

This pull request fails (merged cbc514f9 into 10337cb).

This pull request passes (merged a41ad9e into 290fd4e).

This pull request passes (merged 81f63d8 into 290fd4e).

This pull request fails (merged 9dbeccf into 290fd4e).

Owner

ugisozols commented Jun 22, 2012

I would like for the dialog to close after I upload images because currently it just stays open showing success message and you have to close it yourself.

Refinery CMS Image upload

Contributor

jipiboily commented Jun 23, 2012

@ugisozols should it be closed right away and the flash message should then be showed where it was before?

Owner

ugisozols commented Jun 23, 2012

@jipiboily yes.

Owner

parndt commented Jul 6, 2012

I'm guessing the last request added somewhat of a challenge?

Contributor

jipiboily commented Jul 12, 2012

@parndt things in my personal life kept me away from coding...I will do that really soon. I might have to play with the flash header to append all file names to the message...

Owner

parndt commented Jul 12, 2012

@jipiboily I believe so yes! :-)

Contributor

jipiboily commented Jul 13, 2012

I finally removed the flash, it felt too hacky.

It will automatically close the dialog after a successful upload and will show that: http://cl.ly/1f0h2W2g1V450w3M3L0T

Is that what you imagined?

This pull request passes (merged 0e3037d into c3c3dd7).

Owner

ugisozols commented Jul 13, 2012

@jipiboily yes.

Contributor

robyurkowski commented Jul 13, 2012

@jipiboily What does it show when you upload multiple while inserting?

Contributor

jipiboily commented Jul 13, 2012

@robyurkowski not sure to understand what you mean. You mean during the process? You want me to shoot a video of the process? :)

Contributor

robyurkowski commented Jul 13, 2012

Like if I'm uploading while inserting into a page. What is the behaviour then?

Contributor

jipiboily commented Jul 16, 2012

Hum, it is closing the dialog now...which is unexpected in this specific use case, and unwanted.

Contributor

jipiboily commented Jul 24, 2012

Two choices guys from here. Or we don't close the dialog automatically in neither of the situations, or we don't close it when inserting an image into a page. What would you prefer?

Owner

ugisozols commented Jul 27, 2012

@jipiboily can you make it behave the same as it is now? Thanks.

Contributor

jipiboily commented Aug 1, 2012

Sure, I'll do!

This pull request passes (merged c4873b1 into 765174c).

Contributor

jipiboily commented Aug 2, 2012

Hi guys! I made it working mostly like it is now as far as I know. If I missed something (I didn't use Refinery that much), do not hesitate to tell me!

Cheers

This pull request fails (merged 4b3a41d into 765174c).

Owner

ugisozols commented Aug 9, 2012

Hey @jipiboily,

Just tried out this and it works great! Can you do 2 more things?

  • Remove flash message from the dialog window - while you upload images it shows status and after the upload is complete it shows flash message in the same window. Right after that it closes window and redisplays flash message in the main page which is enough.
  • Add changelog entry

Thanks!

Owner

parndt commented Aug 20, 2012

I would love to get this merged soon as it's a nice feature. @ugisozols @robyurkowski maybe we could implement the last couple of items?

ugisozols referenced this pull request Aug 27, 2012

Merged

ajax image upload #1903

Owner

ugisozols commented Aug 27, 2012

I did the changes I wanted and pushed to a new branch because I couldn't open a PR against @jipiboily repo.

Please review.

ugisozols closed this Aug 27, 2012

@ugisozols ugisozols added a commit that referenced this pull request Aug 27, 2012

@ugisozols @parndt ugisozols + parndt Add changelog entry about #1739. 77d5016
Contributor

jipiboily commented Aug 27, 2012

I just got back from 3 weeks of vacations. Thanks for putting the last touch @ugisozols!

Contributor

jokklan commented Oct 1, 2012

Is someone currently working on this?

Owner

ugisozols commented Oct 1, 2012

Nope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment