From 7db4a91d78bc24b3956797e2d9792077a04010ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 13:19:02 +0200 Subject: [PATCH 01/22] Remove duplicate nested image partial It had almost exactly the same code as the standard nested image partial, and its variations were insignificant. --- app/views/admin/milestones/_form.html.erb | 2 +- app/views/admin/poll/polls/_form.html.erb | 4 +-- app/views/images/_admin_image.html.erb | 40 ----------------------- config/locales/en/images.yml | 2 -- config/locales/es/images.yml | 2 -- 5 files changed, 3 insertions(+), 47 deletions(-) delete mode 100644 app/views/images/_admin_image.html.erb diff --git a/app/views/admin/milestones/_form.html.erb b/app/views/admin/milestones/_form.html.erb index 07fe07e8b..8c56eaaad 100644 --- a/app/views/admin/milestones/_form.html.erb +++ b/app/views/admin/milestones/_form.html.erb @@ -27,7 +27,7 @@
<%= f.date_field :publication_date %> - <%= render "images/admin_image", imageable: @milestone, f: f %> + <%= render "images/nested_image", imageable: @milestone, f: f %>
<%= render "documents/nested_documents", documentable: @milestone, f: f %> diff --git a/app/views/admin/poll/polls/_form.html.erb b/app/views/admin/poll/polls/_form.html.erb index b3ce9eaf3..902a10e0a 100644 --- a/app/views/admin/poll/polls/_form.html.erb +++ b/app/views/admin/poll/polls/_form.html.erb @@ -32,8 +32,8 @@
-
- <%= render "images/admin_image", imageable: @poll, f: f %> +
+ <%= render "images/nested_image", imageable: @poll, f: f %>
diff --git a/app/views/images/_admin_image.html.erb b/app/views/images/_admin_image.html.erb deleted file mode 100644 index 95ce0c752..000000000 --- a/app/views/images/_admin_image.html.erb +++ /dev/null @@ -1,40 +0,0 @@ -
-
- <%= f.label :image, t("images.form.admin_title") %> - - <%= link_to_add_association t("images.form.add_new_image"), f, :image, - force_non_association_create: true, - partial: "images/image_fields", - id: "new_image_link", - class: "button hollow", - render_options: { - locals: { imageable: imageable } - }, - data: { - association_insertion_node: "#nested-image", - association_insertion_method: "append" - } %> - - <%= render_image(f.object.image, :thumb, false) if f.object.image %> - -
- <%= f.fields_for :image do |image_builder| %> - -
- <%= image_builder.hidden_field :id %> - <%= image_builder.hidden_field :user_id, value: current_user.id %> - <%= image_builder.hidden_field :cached_attachment %> - - <%= image_builder.text_field :title, placeholder: t("images.form.title_placeholder"), label: "#{t("images.form.admin_alt_text")}" %> - -
-
- <%= render_image_attachment(image_builder, imageable, image_builder.object) %> -
-
-
- - <% end %> -
-
-
diff --git a/config/locales/en/images.yml b/config/locales/en/images.yml index 480b3e232..02ee8d843 100644 --- a/config/locales/en/images.yml +++ b/config/locales/en/images.yml @@ -8,8 +8,6 @@ en: note: "You can upload one image of following content types: %{accepted_content_types}, up to %{max_file_size} MB." add_new_image: Add image title_placeholder: Add a descriptive title for the image - admin_title: "Image" - admin_alt_text: "Alternative text for the image" actions: destroy: notice: Image was deleted successfully. diff --git a/config/locales/es/images.yml b/config/locales/es/images.yml index edc299e1e..ed197fa7d 100644 --- a/config/locales/es/images.yml +++ b/config/locales/es/images.yml @@ -7,8 +7,6 @@ es: delete_button: Eliminar imagen note: "Puedes subir una imagen en los formatos: %{accepted_content_types}, y de hasta %{max_file_size} MB por archivo." add_new_image: Añadir imagen - admin_title: "Imagen" - admin_alt_text: "Texto alternativo para la imagen" actions: destroy: notice: La imagen se ha eliminado correctamente. From a7ed1f9a37ad800bb815cbaeca4c9789f45f717b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 13:34:54 +0200 Subject: [PATCH 02/22] Remove unused errors on attachment methods They aren't used since commit f8d78ec4a. --- app/helpers/documents_helper.rb | 4 ---- app/helpers/images_helper.rb | 4 ---- 2 files changed, 8 deletions(-) diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index 574e971b9..79b052a47 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -3,10 +3,6 @@ module DocumentsHelper document.attachment_file_name end - def document_errors_on_attachment(document) - document.errors[:attachment].join(", ") if document.errors.key?(:attachment) - end - def render_destroy_document_link(builder, document) if !document.persisted? && document.cached_attachment.present? link_to t("documents.form.delete_button"), diff --git a/app/helpers/images_helper.rb b/app/helpers/images_helper.rb index b265e10ad..7777e0ec3 100644 --- a/app/helpers/images_helper.rb +++ b/app/helpers/images_helper.rb @@ -13,10 +13,6 @@ module ImagesHelper image.attachment_file_name end - def image_errors_on_attachment(image) - image.errors[:attachment].join(", ") if image.errors.key?(:attachment) - end - def image_class(image) image.persisted? ? "persisted-image" : "cached-image" end From d58c2f83237072c1b4f8952daaa2c81778df2023 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 13:37:25 +0200 Subject: [PATCH 03/22] Move image fields partial to a component --- .../images/fields_component.html.erb | 32 ++++++++++ app/components/images/fields_component.rb | 60 +++++++++++++++++++ app/helpers/imageables_helper.rb | 10 ---- app/helpers/images_helper.rb | 39 ------------ app/views/images/_image_fields.html.erb | 33 +--------- app/views/images/_nested_image.html.erb | 2 +- 6 files changed, 94 insertions(+), 82 deletions(-) create mode 100644 app/components/images/fields_component.html.erb create mode 100644 app/components/images/fields_component.rb diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb new file mode 100644 index 000000000..991554381 --- /dev/null +++ b/app/components/images/fields_component.html.erb @@ -0,0 +1,32 @@ +
+ <%= f.hidden_field :id %> + <%= f.hidden_field :user_id, value: current_user.id %> + <%= f.hidden_field :cached_attachment %> + +
+ <%= f.text_field :title, placeholder: t("images.form.title_placeholder") %> +
+ + <%= render_image(f.object, :thumb, false) if f.object.attachment.exists? %> + +
+
+ <%= render_image_attachment(f.object) %> +
+
+ <%= render_destroy_image_link(f.object) %> +
+
+ +
+

+ <%= image_attachment_file_name(f.object) %> +

+
+ +
+
+
+ +
+
diff --git a/app/components/images/fields_component.rb b/app/components/images/fields_component.rb new file mode 100644 index 000000000..ab35baf9e --- /dev/null +++ b/app/components/images/fields_component.rb @@ -0,0 +1,60 @@ +class Images::FieldsComponent < ApplicationComponent + attr_reader :f, :imageable + delegate :current_user, :render_image, to: :helpers + + def initialize(f, imageable:) + @f = f + @imageable = imageable + end + + private + + def image_attachment_file_name(image) + image.attachment_file_name + end + + def render_destroy_image_link(image) + if !image.persisted? && image.cached_attachment.present? + link_to t("images.form.delete_button"), + direct_upload_destroy_path( + "direct_upload[resource_type]": image.imageable_type, + "direct_upload[resource_id]": image.imageable_id, + "direct_upload[resource_relation]": "image", + "direct_upload[cached_attachment]": image.cached_attachment + ), + method: :delete, + remote: true, + class: "delete remove-cached-attachment" + else + link_to_remove_association t("images.form.delete_button"), f, class: "delete remove-image" + end + end + + def render_image_attachment(image) + klass = image.persisted? || image.cached_attachment.present? ? " hide" : "" + f.file_field :attachment, + label_options: { class: "button hollow #{klass}" }, + accept: imageable_accepted_content_types_extensions, + class: "js-image-attachment", + data: { + url: image_direct_upload_path, + nested_image: true + } + end + + def image_direct_upload_path + direct_uploads_path("direct_upload[resource_type]": imageable.class.name, + "direct_upload[resource_id]": imageable.id, + "direct_upload[resource_relation]": "image") + end + + def imageable_accepted_content_types_extensions + Setting.accepted_content_types_for("images").map do |content_type| + if content_type == "jpg" + ".jpg,.jpeg" + else + ".#{content_type}" + end + end.join(",") + end +end diff --git a/app/helpers/imageables_helper.rb b/app/helpers/imageables_helper.rb index 16b49caf9..b1734b4e2 100644 --- a/app/helpers/imageables_helper.rb +++ b/app/helpers/imageables_helper.rb @@ -11,16 +11,6 @@ module ImageablesHelper Setting["uploads.images.content_types"]&.split(" ") || ["image/jpeg"] end - def imageable_accepted_content_types_extensions - Setting.accepted_content_types_for("images").map do |content_type| - if content_type == "jpg" - ".jpg,.jpeg" - else - ".#{content_type}" - end - end.join(",") - end - def imageable_humanized_accepted_content_types Setting.accepted_content_types_for("images").join(", ") end diff --git a/app/helpers/images_helper.rb b/app/helpers/images_helper.rb index 7777e0ec3..5f992a6b5 100644 --- a/app/helpers/images_helper.rb +++ b/app/helpers/images_helper.rb @@ -9,53 +9,14 @@ module ImagesHelper end end - def image_attachment_file_name(image) - image.attachment_file_name - end - def image_class(image) image.persisted? ? "persisted-image" : "cached-image" end - def render_destroy_image_link(builder, image) - if !image.persisted? && image.cached_attachment.present? - link_to t("images.form.delete_button"), - direct_upload_destroy_path( - "direct_upload[resource_type]": image.imageable_type, - "direct_upload[resource_id]": image.imageable_id, - "direct_upload[resource_relation]": "image", - "direct_upload[cached_attachment]": image.cached_attachment - ), - method: :delete, - remote: true, - class: "delete remove-cached-attachment" - else - link_to_remove_association t("images.form.delete_button"), builder, class: "delete remove-image" - end - end - - def render_image_attachment(builder, imageable, image) - klass = image.persisted? || image.cached_attachment.present? ? " hide" : "" - builder.file_field :attachment, - label_options: { class: "button hollow #{klass}" }, - accept: imageable_accepted_content_types_extensions, - class: "js-image-attachment", - data: { - url: image_direct_upload_path(imageable), - nested_image: true - } - end - def render_image(image, version, show_caption = true) version = image.persisted? ? version : :original render "images/image", image: image, version: version, show_caption: show_caption end - - def image_direct_upload_path(imageable) - direct_uploads_path("direct_upload[resource_type]": imageable.class.name, - "direct_upload[resource_id]": imageable.id, - "direct_upload[resource_relation]": "image") - end end diff --git a/app/views/images/_image_fields.html.erb b/app/views/images/_image_fields.html.erb index 7fbfb0f01..0c3134c2a 100644 --- a/app/views/images/_image_fields.html.erb +++ b/app/views/images/_image_fields.html.erb @@ -1,32 +1 @@ -
- <%= f.hidden_field :id %> - <%= f.hidden_field :user_id, value: current_user.id %> - <%= f.hidden_field :cached_attachment %> - -
- <%= f.text_field :title, placeholder: t("images.form.title_placeholder") %> -
- - <%= render_image(f.object, :thumb, false) if f.object.attachment.exists? %> - -
-
- <%= render_image_attachment(f, imageable, f.object) %> -
-
- <%= render_destroy_image_link(f, f.object) %> -
-
- -
-

- <%= image_attachment_file_name(f.object) %> -

-
- -
-
-
- -
-
+<%= render Images::FieldsComponent.new(f, imageable: imageable) %> diff --git a/app/views/images/_nested_image.html.erb b/app/views/images/_nested_image.html.erb index fc8e6cc62..a3adf08ba 100644 --- a/app/views/images/_nested_image.html.erb +++ b/app/views/images/_nested_image.html.erb @@ -5,7 +5,7 @@
<%= f.fields_for image_fields do |image_builder| %> - <%= render "images/image_fields", f: image_builder, imageable: imageable %> + <%= render Images::FieldsComponent.new(image_builder, imageable: imageable) %> <% end %>
From 810814486c44ce5b32530691942ff030efa4fd0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 13:41:12 +0200 Subject: [PATCH 04/22] Move document fields partial to a component --- .../documents/fields_component.html.erb | 31 +++++++++++ app/components/documents/fields_component.rb | 53 +++++++++++++++++++ app/helpers/documentables_helper.rb | 4 -- app/helpers/documents_helper.rb | 39 -------------- app/views/documents/_document_fields.html.erb | 32 +---------- .../documents/_nested_documents.html.erb | 2 +- 6 files changed, 86 insertions(+), 75 deletions(-) create mode 100644 app/components/documents/fields_component.html.erb create mode 100644 app/components/documents/fields_component.rb diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb new file mode 100644 index 000000000..724711eb1 --- /dev/null +++ b/app/components/documents/fields_component.html.erb @@ -0,0 +1,31 @@ +
+ <%= f.hidden_field :id %> + <%= f.hidden_field :user_id, value: current_user.id %> + <%= f.hidden_field :cached_attachment %> + +
+ <%= f.text_field :title, placeholder: t("documents.form.title_placeholder") %> +
+ +
+
+ <%= render_attachment(f.object) %> +
+
+ <%= render_destroy_document_link(f.object) %> +
+
+ +
+

+ <%= document_attachment_file_name(f.object) %> +

+
+ +
+
+
+ +
+ +
diff --git a/app/components/documents/fields_component.rb b/app/components/documents/fields_component.rb new file mode 100644 index 000000000..b5f2384eb --- /dev/null +++ b/app/components/documents/fields_component.rb @@ -0,0 +1,53 @@ +class Documents::FieldsComponent < ApplicationComponent + attr_reader :f + delegate :current_user, to: :helpers + + def initialize(f) + @f = f + end + + private + + def document_attachment_file_name(document) + document.attachment_file_name + end + + def render_destroy_document_link(document) + if !document.persisted? && document.cached_attachment.present? + link_to t("documents.form.delete_button"), + direct_upload_destroy_path( + "direct_upload[resource_type]": document.documentable_type, + "direct_upload[resource_id]": document.documentable_id, + "direct_upload[resource_relation]": "documents", + "direct_upload[cached_attachment]": document.cached_attachment + ), + method: :delete, + remote: true, + class: "delete remove-cached-attachment" + else + link_to_remove_association document.new_record? ? t("documents.form.cancel_button") : t("documents.form.delete_button"), f, class: "delete remove-document" + end + end + + def render_attachment(document) + klass = document.persisted? || document.cached_attachment.present? ? " hide" : "" + f.file_field :attachment, + label_options: { class: "button hollow #{klass}" }, + accept: accepted_content_types_extensions(document.documentable_type.constantize), + class: "js-document-attachment", + data: { + url: document_direct_upload_path(document), + nested_document: true + } + end + + def document_direct_upload_path(document) + direct_uploads_path("direct_upload[resource_type]": document.documentable_type, + "direct_upload[resource_id]": document.documentable_id, + "direct_upload[resource_relation]": "documents") + end + + def accepted_content_types_extensions(documentable_class) + Setting.accepted_content_types_for("documents").map { |content_type| ".#{content_type}" }.join(",") + end +end diff --git a/app/helpers/documentables_helper.rb b/app/helpers/documentables_helper.rb index 74d7ccf0d..0b0a5e551 100644 --- a/app/helpers/documentables_helper.rb +++ b/app/helpers/documentables_helper.rb @@ -15,10 +15,6 @@ module DocumentablesHelper documentable_class.accepted_content_types end - def accepted_content_types_extensions(documentable_class) - Setting.accepted_content_types_for("documents").map { |content_type| ".#{content_type}" }.join(",") - end - def documentable_humanized_accepted_content_types(documentable_class) Setting.accepted_content_types_for("documents").join(", ") end diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index 79b052a47..3ff564a03 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -1,43 +1,4 @@ module DocumentsHelper - def document_attachment_file_name(document) - document.attachment_file_name - end - - def render_destroy_document_link(builder, document) - if !document.persisted? && document.cached_attachment.present? - link_to t("documents.form.delete_button"), - direct_upload_destroy_path( - "direct_upload[resource_type]": document.documentable_type, - "direct_upload[resource_id]": document.documentable_id, - "direct_upload[resource_relation]": "documents", - "direct_upload[cached_attachment]": document.cached_attachment - ), - method: :delete, - remote: true, - class: "delete remove-cached-attachment" - else - link_to_remove_association document.new_record? ? t("documents.form.cancel_button") : t("documents.form.delete_button"), builder, class: "delete remove-document" - end - end - - def render_attachment(builder, document) - klass = document.persisted? || document.cached_attachment.present? ? " hide" : "" - builder.file_field :attachment, - label_options: { class: "button hollow #{klass}" }, - accept: accepted_content_types_extensions(document.documentable_type.constantize), - class: "js-document-attachment", - data: { - url: document_direct_upload_path(document), - nested_document: true - } - end - - def document_direct_upload_path(document) - direct_uploads_path("direct_upload[resource_type]": document.documentable_type, - "direct_upload[resource_id]": document.documentable_id, - "direct_upload[resource_relation]": "documents") - end - def document_item_link(document) info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}" diff --git a/app/views/documents/_document_fields.html.erb b/app/views/documents/_document_fields.html.erb index 8f532c71b..6e452d9b2 100644 --- a/app/views/documents/_document_fields.html.erb +++ b/app/views/documents/_document_fields.html.erb @@ -1,31 +1 @@ -
- <%= f.hidden_field :id %> - <%= f.hidden_field :user_id, value: current_user.id %> - <%= f.hidden_field :cached_attachment %> - -
- <%= f.text_field :title, placeholder: t("documents.form.title_placeholder") %> -
- -
-
- <%= render_attachment(f, f.object) %> -
-
- <%= render_destroy_document_link(f, f.object) %> -
-
- -
-

- <%= document_attachment_file_name(f.object) %> -

-
- -
-
-
- -
- -
+<%= render Documents::FieldsComponent.new(f) %> diff --git a/app/views/documents/_nested_documents.html.erb b/app/views/documents/_nested_documents.html.erb index d4f80dd8e..e706b0446 100644 --- a/app/views/documents/_nested_documents.html.erb +++ b/app/views/documents/_nested_documents.html.erb @@ -4,7 +4,7 @@
<%= f.fields_for :documents do |documents_builder| %> - <%= render "documents/document_fields", f: documents_builder %> + <%= render Documents::FieldsComponent.new(documents_builder) %> <% end %>
From 5cf96ba03d4df15ef2161017c021018e9f516d7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 13:43:30 +0200 Subject: [PATCH 05/22] Extract methods to get objects in attachment forms --- .../documents/fields_component.html.erb | 8 ++++---- app/components/documents/fields_component.rb | 18 +++++++++++------- .../images/fields_component.html.erb | 10 +++++----- app/components/images/fields_component.rb | 10 +++++++--- 4 files changed, 27 insertions(+), 19 deletions(-) diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb index 724711eb1..9e8f3b5fc 100644 --- a/app/components/documents/fields_component.html.erb +++ b/app/components/documents/fields_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= f.hidden_field :id %> <%= f.hidden_field :user_id, value: current_user.id %> <%= f.hidden_field :cached_attachment %> @@ -9,16 +9,16 @@
- <%= render_attachment(f.object) %> + <%= render_attachment %>
- <%= render_destroy_document_link(f.object) %> + <%= render_destroy_document_link %>

- <%= document_attachment_file_name(f.object) %> + <%= document_attachment_file_name %>

diff --git a/app/components/documents/fields_component.rb b/app/components/documents/fields_component.rb index b5f2384eb..3e285ccf7 100644 --- a/app/components/documents/fields_component.rb +++ b/app/components/documents/fields_component.rb @@ -8,11 +8,15 @@ class Documents::FieldsComponent < ApplicationComponent private - def document_attachment_file_name(document) + def document + f.object + end + + def document_attachment_file_name document.attachment_file_name end - def render_destroy_document_link(document) + def render_destroy_document_link if !document.persisted? && document.cached_attachment.present? link_to t("documents.form.delete_button"), direct_upload_destroy_path( @@ -29,25 +33,25 @@ class Documents::FieldsComponent < ApplicationComponent end end - def render_attachment(document) + def render_attachment klass = document.persisted? || document.cached_attachment.present? ? " hide" : "" f.file_field :attachment, label_options: { class: "button hollow #{klass}" }, - accept: accepted_content_types_extensions(document.documentable_type.constantize), + accept: accepted_content_types_extensions, class: "js-document-attachment", data: { - url: document_direct_upload_path(document), + url: document_direct_upload_path, nested_document: true } end - def document_direct_upload_path(document) + def document_direct_upload_path direct_uploads_path("direct_upload[resource_type]": document.documentable_type, "direct_upload[resource_id]": document.documentable_id, "direct_upload[resource_relation]": "documents") end - def accepted_content_types_extensions(documentable_class) + def accepted_content_types_extensions Setting.accepted_content_types_for("documents").map { |content_type| ".#{content_type}" }.join(",") end end diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb index 991554381..5e922c788 100644 --- a/app/components/images/fields_component.html.erb +++ b/app/components/images/fields_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= f.hidden_field :id %> <%= f.hidden_field :user_id, value: current_user.id %> <%= f.hidden_field :cached_attachment %> @@ -7,20 +7,20 @@ <%= f.text_field :title, placeholder: t("images.form.title_placeholder") %>
- <%= render_image(f.object, :thumb, false) if f.object.attachment.exists? %> + <%= render_image(image, :thumb, false) if image.attachment.exists? %>
- <%= render_image_attachment(f.object) %> + <%= render_image_attachment %>
- <%= render_destroy_image_link(f.object) %> + <%= render_destroy_image_link %>

- <%= image_attachment_file_name(f.object) %> + <%= image_attachment_file_name %>

diff --git a/app/components/images/fields_component.rb b/app/components/images/fields_component.rb index ab35baf9e..088b1e585 100644 --- a/app/components/images/fields_component.rb +++ b/app/components/images/fields_component.rb @@ -9,11 +9,15 @@ class Images::FieldsComponent < ApplicationComponent private - def image_attachment_file_name(image) + def image + f.object + end + + def image_attachment_file_name image.attachment_file_name end - def render_destroy_image_link(image) + def render_destroy_image_link if !image.persisted? && image.cached_attachment.present? link_to t("images.form.delete_button"), direct_upload_destroy_path( @@ -30,7 +34,7 @@ class Images::FieldsComponent < ApplicationComponent end end - def render_image_attachment(image) + def render_image_attachment klass = image.persisted? || image.cached_attachment.present? ? " hide" : "" f.file_field :attachment, label_options: { class: "button hollow #{klass}" }, From afbd1fec371316dc79e4e5e21d65954fe25d1b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 14:27:38 +0200 Subject: [PATCH 06/22] Remove unused methods to attach files in tests These methods aren't used since commits eef8ad1b7 and 2993ef87. --- spec/shared/system/documentable.rb | 9 --------- spec/shared/system/imageable.rb | 11 ----------- 2 files changed, 20 deletions(-) diff --git a/spec/shared/system/documentable.rb b/spec/shared/system/documentable.rb index 062d978c0..296da2bda 100644 --- a/spec/shared/system/documentable.rb +++ b/spec/shared/system/documentable.rb @@ -134,12 +134,3 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, end end end - -def attach_document(path, success = true) - attach_file :document_attachment, path, make_visible: true - if success - expect(page).to have_css ".loading-bar.complete" - else - expect(page).to have_css ".loading-bar.errors" - end -end diff --git a/spec/shared/system/imageable.rb b/spec/shared/system/imageable.rb index 6b39c17ea..59e08af32 100644 --- a/spec/shared/system/imageable.rb +++ b/spec/shared/system/imageable.rb @@ -30,14 +30,3 @@ shared_examples "imageable" do |imageable_factory_name, imageable_path, imageabl end end end - -def attach_image(path, success = true) - image = find(".image") - image_input = image.find("input[type=file]", visible: false) - attach_file image_input[:id], path, make_visible: true - if success - expect(page).to have_css ".loading-bar.complete" - else - expect(page).to have_css ".loading-bar.errors" - end -end From 8116e75aee14d35648f59fe4cf77dd0275acded1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 16:16:39 +0200 Subject: [PATCH 07/22] Simplify showing/hiding attached file name Note we have to render the `

` tag in one line because at the time of writing browsers don't consider elements with whitespace inside as empty [1]. [1] https://developer.mozilla.org/en-US/docs/Web/CSS/:empty --- app/assets/javascripts/documentable.js | 2 -- app/assets/javascripts/imageable.js | 2 -- app/assets/stylesheets/mixins/uploads.scss | 4 ++++ app/components/documents/fields_component.html.erb | 4 +--- app/components/images/fields_component.html.erb | 4 +--- 5 files changed, 6 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index b6b85ba8c..1a7cf33ff 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -92,7 +92,6 @@ }, clearFilename: function(data) { $(data.fileNameContainer).text(""); - $(data.fileNameContainer).hide(); }, clearInputErrors: function(data) { $(data.errorContainer).find("small.error").remove(); @@ -102,7 +101,6 @@ }, setFilename: function(data, file_name) { $(data.fileNameContainer).text(file_name); - $(data.fileNameContainer).show(); }, setProgressBar: function(data, klass) { $(data.progressBar).find(".loading-bar").addClass(klass); diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 7f0773907..5b784f475 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -92,7 +92,6 @@ }, clearFilename: function(data) { $(data.fileNameContainer).text(""); - $(data.fileNameContainer).hide(); }, clearInputErrors: function(data) { $(data.errorContainer).find("small.error").remove(); @@ -105,7 +104,6 @@ }, setFilename: function(data, file_name) { $(data.fileNameContainer).text(file_name); - $(data.fileNameContainer).show(); }, setProgressBar: function(data, klass) { $(data.progressBar).find(".loading-bar").addClass(klass); diff --git a/app/assets/stylesheets/mixins/uploads.scss b/app/assets/stylesheets/mixins/uploads.scss index f7fd718fc..1aa737553 100644 --- a/app/assets/stylesheets/mixins/uploads.scss +++ b/app/assets/stylesheets/mixins/uploads.scss @@ -42,6 +42,10 @@ .file-name { margin-top: 0; + + &:empty { + display: none; + } } .loading-bar { diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb index 9e8f3b5fc..c5dabe5c2 100644 --- a/app/components/documents/fields_component.html.erb +++ b/app/components/documents/fields_component.html.erb @@ -17,9 +17,7 @@

-

- <%= document_attachment_file_name %> -

+

<%= document_attachment_file_name %>

diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb index 5e922c788..5681bca1a 100644 --- a/app/components/images/fields_component.html.erb +++ b/app/components/images/fields_component.html.erb @@ -19,9 +19,7 @@
-

- <%= image_attachment_file_name %> -

+

<%= image_attachment_file_name %>

From 21fee9cff52f0d397de2619e8d0064538c0ce4a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 20 Jun 2021 19:17:29 +0200 Subject: [PATCH 08/22] Simplify method names in image/document fields We were using long, unique names because these methods used to be helper methods. Helper methods should have unique names because otherwise one method would overwrite the other. Now that we're using components, we can omit the `image_` and `document_` prefixes. --- app/components/documents/fields_component.html.erb | 6 +++--- app/components/documents/fields_component.rb | 10 +++++----- app/components/images/fields_component.html.erb | 6 +++--- app/components/images/fields_component.rb | 14 +++++++------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb index c5dabe5c2..4dc0c42ff 100644 --- a/app/components/documents/fields_component.html.erb +++ b/app/components/documents/fields_component.html.erb @@ -9,15 +9,15 @@
- <%= render_attachment %> + <%= file_field %>
- <%= render_destroy_document_link %> + <%= destroy_link %>
-

<%= document_attachment_file_name %>

+

<%= file_name %>

diff --git a/app/components/documents/fields_component.rb b/app/components/documents/fields_component.rb index 3e285ccf7..4d503d912 100644 --- a/app/components/documents/fields_component.rb +++ b/app/components/documents/fields_component.rb @@ -12,11 +12,11 @@ class Documents::FieldsComponent < ApplicationComponent f.object end - def document_attachment_file_name + def file_name document.attachment_file_name end - def render_destroy_document_link + def destroy_link if !document.persisted? && document.cached_attachment.present? link_to t("documents.form.delete_button"), direct_upload_destroy_path( @@ -33,19 +33,19 @@ class Documents::FieldsComponent < ApplicationComponent end end - def render_attachment + def file_field klass = document.persisted? || document.cached_attachment.present? ? " hide" : "" f.file_field :attachment, label_options: { class: "button hollow #{klass}" }, accept: accepted_content_types_extensions, class: "js-document-attachment", data: { - url: document_direct_upload_path, + url: direct_upload_path, nested_document: true } end - def document_direct_upload_path + def direct_upload_path direct_uploads_path("direct_upload[resource_type]": document.documentable_type, "direct_upload[resource_id]": document.documentable_id, "direct_upload[resource_relation]": "documents") diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb index 5681bca1a..bff9555f1 100644 --- a/app/components/images/fields_component.html.erb +++ b/app/components/images/fields_component.html.erb @@ -11,15 +11,15 @@
- <%= render_image_attachment %> + <%= file_field %>
- <%= render_destroy_image_link %> + <%= destroy_link %>
-

<%= image_attachment_file_name %>

+

<%= file_name %>

diff --git a/app/components/images/fields_component.rb b/app/components/images/fields_component.rb index 088b1e585..0cc91d28c 100644 --- a/app/components/images/fields_component.rb +++ b/app/components/images/fields_component.rb @@ -13,11 +13,11 @@ class Images::FieldsComponent < ApplicationComponent f.object end - def image_attachment_file_name + def file_name image.attachment_file_name end - def render_destroy_image_link + def destroy_link if !image.persisted? && image.cached_attachment.present? link_to t("images.form.delete_button"), direct_upload_destroy_path( @@ -34,25 +34,25 @@ class Images::FieldsComponent < ApplicationComponent end end - def render_image_attachment + def file_field klass = image.persisted? || image.cached_attachment.present? ? " hide" : "" f.file_field :attachment, label_options: { class: "button hollow #{klass}" }, - accept: imageable_accepted_content_types_extensions, + accept: accepted_content_types_extensions, class: "js-image-attachment", data: { - url: image_direct_upload_path, + url: direct_upload_path, nested_image: true } end - def image_direct_upload_path + def direct_upload_path direct_uploads_path("direct_upload[resource_type]": imageable.class.name, "direct_upload[resource_id]": imageable.id, "direct_upload[resource_relation]": "image") end - def imageable_accepted_content_types_extensions + def accepted_content_types_extensions Setting.accepted_content_types_for("images").map do |content_type| if content_type == "jpg" ".jpg,.jpeg" From 3073fee4c8928474a698650429cac6d5ab8a29ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 20:14:01 +0200 Subject: [PATCH 09/22] Remove unused documentable class method It isn't used since commit 6c1d828a6. --- app/helpers/documentables_helper.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/helpers/documentables_helper.rb b/app/helpers/documentables_helper.rb index 0b0a5e551..eae1e1e1f 100644 --- a/app/helpers/documentables_helper.rb +++ b/app/helpers/documentables_helper.rb @@ -1,8 +1,4 @@ module DocumentablesHelper - def documentable_class(documentable) - documentable.class.name.parameterize(separator: "_") - end - def max_documents_allowed(documentable) documentable.class.max_documents_allowed end From ac495e752393955806a1d650646ede72a5e9e500 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 20:41:26 +0200 Subject: [PATCH 10/22] Remove unnecessary code in poll images controller We're already loading the answer with a `before_action`. --- .../admin/poll/questions/answers/images_controller.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/admin/poll/questions/answers/images_controller.rb b/app/controllers/admin/poll/questions/answers/images_controller.rb index 37e4cfcc8..e39a8bc66 100644 --- a/app/controllers/admin/poll/questions/answers/images_controller.rb +++ b/app/controllers/admin/poll/questions/answers/images_controller.rb @@ -7,11 +7,9 @@ class Admin::Poll::Questions::Answers::ImagesController < Admin::Poll::BaseContr end def new - @answer = ::Poll::Question::Answer.find(params[:answer_id]) end def create - @answer = ::Poll::Question::Answer.find(params[:answer_id]) @answer.attributes = images_params if @answer.save From 629df5ab9bef936359cb3fc2ba4eacba334922bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 20:45:57 +0200 Subject: [PATCH 11/22] Simplify getting imageable/documentable in forms The imageable/documentable object is always the object the form builder is based on; since we're already passing the form builder, we don't have to pass the object as well. The only exception are the poll answers. In this case, we're passing a new answer as the object. That's OK; the same hack that we're using to send the data to the answer URL without displaying existing attachments causes the form to keep working the same way. --- app/components/admin/budget_phases/form_component.html.erb | 2 +- app/components/admin/budgets/form_component.html.erb | 2 +- app/components/budgets/investments/form_component.html.erb | 4 ++-- app/components/proposals/form_component.html.erb | 4 ++-- app/views/admin/dashboard/actions/_form.html.erb | 2 +- app/views/admin/legislation/processes/_form.html.erb | 4 ++-- app/views/admin/milestones/_form.html.erb | 4 ++-- app/views/admin/poll/polls/_form.html.erb | 2 +- app/views/admin/poll/questions/answers/documents.html.erb | 2 +- app/views/admin/poll/questions/answers/images/new.html.erb | 2 +- app/views/admin/widget/cards/_form.html.erb | 2 +- app/views/documents/_nested_documents.html.erb | 2 ++ app/views/images/_nested_image.html.erb | 1 + app/views/legislation/proposals/_form.html.erb | 4 ++-- 14 files changed, 20 insertions(+), 17 deletions(-) diff --git a/app/components/admin/budget_phases/form_component.html.erb b/app/components/admin/budget_phases/form_component.html.erb index 3fcaab81c..45e8b65d4 100644 --- a/app/components/admin/budget_phases/form_component.html.erb +++ b/app/components/admin/budget_phases/form_component.html.erb @@ -57,7 +57,7 @@ <% if feature?(:allow_images) %>
- <%= render "images/nested_image", imageable: @phase, f: f %> + <%= render "images/nested_image", f: f %>

<%= t("admin.budget_phases.edit.image_description") %>

<% end %> diff --git a/app/components/admin/budgets/form_component.html.erb b/app/components/admin/budgets/form_component.html.erb index 6000b586d..ae286844e 100644 --- a/app/components/admin/budgets/form_component.html.erb +++ b/app/components/admin/budgets/form_component.html.erb @@ -41,7 +41,7 @@ <% if feature?(:allow_images) %>
- <%= render "/images/nested_image", imageable: budget, f: f %> + <%= render "/images/nested_image", f: f %>

<%= t("admin.budgets.edit.image_description") %>

<% end %> diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index 34283dbd8..3705bc10e 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -38,13 +38,13 @@ <% if feature?(:allow_images) %>
- <%= render "images/nested_image", imageable: investment, f: f %> + <%= render "images/nested_image", f: f %>
<% end %> <% if feature?(:allow_attached_documents) %>
- <%= render "documents/nested_documents", documentable: investment, f: f %> + <%= render "documents/nested_documents", f: f %>
<% end %> diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index f1af97081..566391e58 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -39,13 +39,13 @@ <% if feature?(:allow_images) %>
- <%= render "images/nested_image", imageable: proposal, f: f %> + <%= render "images/nested_image", f: f %>
<% end %> <% if feature?(:allow_attached_documents) %>
- <%= render "documents/nested_documents", documentable: proposal, f: f %> + <%= render "documents/nested_documents", f: f %>
<% end %> diff --git a/app/views/admin/dashboard/actions/_form.html.erb b/app/views/admin/dashboard/actions/_form.html.erb index 810726868..178fe0596 100644 --- a/app/views/admin/dashboard/actions/_form.html.erb +++ b/app/views/admin/dashboard/actions/_form.html.erb @@ -62,7 +62,7 @@ <% if feature?(:allow_attached_documents) %>
- <%= render "documents/nested_documents", documentable: dashboard_action, f: f %> + <%= render "documents/nested_documents", f: f %>
<% end %> diff --git a/app/views/admin/legislation/processes/_form.html.erb b/app/views/admin/legislation/processes/_form.html.erb index 98ab0e51c..3094d4ccd 100644 --- a/app/views/admin/legislation/processes/_form.html.erb +++ b/app/views/admin/legislation/processes/_form.html.erb @@ -110,7 +110,7 @@
- <%= render "documents/nested_documents", documentable: @process, f: f %> + <%= render "documents/nested_documents", f: f %>
@@ -118,7 +118,7 @@
- <%= render "images/nested_image", imageable: @process, f: f %> + <%= render "images/nested_image", f: f %>
diff --git a/app/views/admin/milestones/_form.html.erb b/app/views/admin/milestones/_form.html.erb index 8c56eaaad..23314333f 100644 --- a/app/views/admin/milestones/_form.html.erb +++ b/app/views/admin/milestones/_form.html.erb @@ -27,10 +27,10 @@
<%= f.date_field :publication_date %> - <%= render "images/nested_image", imageable: @milestone, f: f %> + <%= render "images/nested_image", f: f %>
- <%= render "documents/nested_documents", documentable: @milestone, f: f %> + <%= render "documents/nested_documents", f: f %>
<%= f.submit nil, class: "button success" %> diff --git a/app/views/admin/poll/polls/_form.html.erb b/app/views/admin/poll/polls/_form.html.erb index 902a10e0a..926a98ce4 100644 --- a/app/views/admin/poll/polls/_form.html.erb +++ b/app/views/admin/poll/polls/_form.html.erb @@ -33,7 +33,7 @@
- <%= render "images/nested_image", imageable: @poll, f: f %> + <%= render "images/nested_image", f: f %>
diff --git a/app/views/admin/poll/questions/answers/documents.html.erb b/app/views/admin/poll/questions/answers/documents.html.erb index 786c38cd9..71d6baeb5 100644 --- a/app/views/admin/poll/questions/answers/documents.html.erb +++ b/app/views/admin/poll/questions/answers/documents.html.erb @@ -15,7 +15,7 @@ <%= render "shared/errors", resource: @answer %>
- <%= render "documents/nested_documents", documentable: @answer, f: f %> + <%= render "documents/nested_documents", f: f %>
diff --git a/app/views/admin/poll/questions/answers/images/new.html.erb b/app/views/admin/poll/questions/answers/images/new.html.erb index a6e401047..d271f2ca6 100644 --- a/app/views/admin/poll/questions/answers/images/new.html.erb +++ b/app/views/admin/poll/questions/answers/images/new.html.erb @@ -5,7 +5,7 @@ <%= render "shared/errors", resource: @answer %>
- <%= render "images/nested_image", imageable: @answer, f: f, image_fields: :images %> + <%= render "images/nested_image", f: f, image_fields: :images %>
<%= f.submit t("admin.questions.answers.images.save_image"), class: "button success" %> diff --git a/app/views/admin/widget/cards/_form.html.erb b/app/views/admin/widget/cards/_form.html.erb index 37676354b..606697f4d 100644 --- a/app/views/admin/widget/cards/_form.html.erb +++ b/app/views/admin/widget/cards/_form.html.erb @@ -45,7 +45,7 @@
- <%= render "images/nested_image", imageable: card, f: f %> + <%= render "images/nested_image", f: f %>
diff --git a/app/views/documents/_nested_documents.html.erb b/app/views/documents/_nested_documents.html.erb index e706b0446..4fd7411cf 100644 --- a/app/views/documents/_nested_documents.html.erb +++ b/app/views/documents/_nested_documents.html.erb @@ -1,3 +1,5 @@ +<% documentable = f.object %> +
<%= f.label :documents, t("documents.form.title") %>

<%= documentables_note(documentable) %>

diff --git a/app/views/images/_nested_image.html.erb b/app/views/images/_nested_image.html.erb index a3adf08ba..4dacbede9 100644 --- a/app/views/images/_nested_image.html.erb +++ b/app/views/images/_nested_image.html.erb @@ -1,4 +1,5 @@ <% image_fields ||= :image %> +<% imageable = f.object %> <%= f.label image_fields, t("images.form.title") %>

<%= imageables_note(imageable) %>

diff --git a/app/views/legislation/proposals/_form.html.erb b/app/views/legislation/proposals/_form.html.erb index f1f8aef59..6e65c5ff1 100644 --- a/app/views/legislation/proposals/_form.html.erb +++ b/app/views/legislation/proposals/_form.html.erb @@ -28,12 +28,12 @@ <% if feature?(:allow_images) %>
- <%= render "images/nested_image", imageable: @proposal, f: f %> + <%= render "images/nested_image", f: f %>
<% end %>
- <%= render "documents/nested_documents", documentable: @proposal, f: f %> + <%= render "documents/nested_documents", f: f %>
From c9113041c0b6d05f18ab511107e222692cf2966c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 21:22:11 +0200 Subject: [PATCH 12/22] Move nested documents partial to a component --- .../documents/nested_component.html.erb | 24 ++++++++++++++++ app/components/documents/nested_component.rb | 28 +++++++++++++++++++ app/helpers/documentables_helper.rb | 14 ---------- .../documents/_nested_documents.html.erb | 27 +----------------- 4 files changed, 53 insertions(+), 40 deletions(-) create mode 100644 app/components/documents/nested_component.html.erb create mode 100644 app/components/documents/nested_component.rb diff --git a/app/components/documents/nested_component.html.erb b/app/components/documents/nested_component.html.erb new file mode 100644 index 000000000..acff7d0e3 --- /dev/null +++ b/app/components/documents/nested_component.html.erb @@ -0,0 +1,24 @@ +
+ <%= f.label :documents, t("documents.form.title") %> +

<%= documentables_note %>

+ +
+ <%= f.fields_for :documents do |documents_builder| %> + <%= render Documents::FieldsComponent.new(documents_builder) %> + <% end %> +
+ + <%= link_to_add_association t("documents.form.add_new_document"), f, :documents, + partial: "documents/document_fields", + id: "new_document_link", + class: "button upload-document + #{"hide" if max_documents_allowed?}", + data: { + association_insertion_node: "#nested-documents", + association_insertion_method: "append" + } %> + +
"> + <%= sanitize(t("documents.max_documents_allowed_reached")) %> +
+
diff --git a/app/components/documents/nested_component.rb b/app/components/documents/nested_component.rb new file mode 100644 index 000000000..14ddd857e --- /dev/null +++ b/app/components/documents/nested_component.rb @@ -0,0 +1,28 @@ +class Documents::NestedComponent < ApplicationComponent + attr_reader :f + delegate :documentable_humanized_accepted_content_types, :max_file_size, to: :helpers + + def initialize(f) + @f = f + end + + private + + def documentable + f.object + end + + def max_documents_allowed + documentable.class.max_documents_allowed + end + + def documentables_note + t "documents.form.note", max_documents_allowed: max_documents_allowed, + accepted_content_types: documentable_humanized_accepted_content_types(documentable.class), + max_file_size: max_file_size(documentable.class) + end + + def max_documents_allowed? + documentable.documents.count >= documentable.class.max_documents_allowed + end +end diff --git a/app/helpers/documentables_helper.rb b/app/helpers/documentables_helper.rb index eae1e1e1f..5dcffacb3 100644 --- a/app/helpers/documentables_helper.rb +++ b/app/helpers/documentables_helper.rb @@ -1,8 +1,4 @@ module DocumentablesHelper - def max_documents_allowed(documentable) - documentable.class.max_documents_allowed - end - def max_file_size(documentable_class) documentable_class.max_file_size / Numeric::MEGABYTE end @@ -14,14 +10,4 @@ module DocumentablesHelper def documentable_humanized_accepted_content_types(documentable_class) Setting.accepted_content_types_for("documents").join(", ") end - - def documentables_note(documentable) - t "documents.form.note", max_documents_allowed: max_documents_allowed(documentable), - accepted_content_types: documentable_humanized_accepted_content_types(documentable.class), - max_file_size: max_file_size(documentable.class) - end - - def max_documents_allowed?(documentable) - documentable.documents.count >= documentable.class.max_documents_allowed - end end diff --git a/app/views/documents/_nested_documents.html.erb b/app/views/documents/_nested_documents.html.erb index 4fd7411cf..229840e83 100644 --- a/app/views/documents/_nested_documents.html.erb +++ b/app/views/documents/_nested_documents.html.erb @@ -1,26 +1 @@ -<% documentable = f.object %> - -
- <%= f.label :documents, t("documents.form.title") %> -

<%= documentables_note(documentable) %>

- -
- <%= f.fields_for :documents do |documents_builder| %> - <%= render Documents::FieldsComponent.new(documents_builder) %> - <% end %> -
- - <%= link_to_add_association t("documents.form.add_new_document"), f, :documents, - partial: "documents/document_fields", - id: "new_document_link", - class: "button upload-document - #{"hide" if max_documents_allowed?(documentable)}", - data: { - association_insertion_node: "#nested-documents", - association_insertion_method: "append" - } %> - -
"> - <%= sanitize(t("documents.max_documents_allowed_reached")) %> -
-
+<%= render Documents::NestedComponent.new(f) %> From a181052f0da9ed2ba8b2767a5cf2ee957bfd9bdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 21:25:57 +0200 Subject: [PATCH 13/22] Move nested images partial to a component --- .../images/nested_component.html.erb | 22 ++++++++++++++++ app/components/images/nested_component.rb | 20 ++++++++++++++ app/helpers/imageables_helper.rb | 5 ---- .../questions/answers/images/new.html.erb | 2 +- app/views/images/_nested_image.html.erb | 26 +------------------ 5 files changed, 44 insertions(+), 31 deletions(-) create mode 100644 app/components/images/nested_component.html.erb create mode 100644 app/components/images/nested_component.rb diff --git a/app/components/images/nested_component.html.erb b/app/components/images/nested_component.html.erb new file mode 100644 index 000000000..fc788c8bd --- /dev/null +++ b/app/components/images/nested_component.html.erb @@ -0,0 +1,22 @@ +<%= f.label image_fields, t("images.form.title") %> +

<%= imageables_note %>

+ +
+ <%= f.fields_for image_fields do |image_builder| %> + <%= render Images::FieldsComponent.new(image_builder, imageable: imageable) %> + <% end %> +
+ +<%= link_to_add_association t("images.form.add_new_image"), f, image_fields, + force_non_association_create: true, + partial: "images/image_fields", + id: "new_image_link", + class: "button upload-image + #{"hide" if image_fields == :image && imageable.image.present?}", + render_options: { + locals: { imageable: imageable } + }, + data: { + association_insertion_node: "#nested-image", + association_insertion_method: "append" + } %> diff --git a/app/components/images/nested_component.rb b/app/components/images/nested_component.rb new file mode 100644 index 000000000..04d4dc5d4 --- /dev/null +++ b/app/components/images/nested_component.rb @@ -0,0 +1,20 @@ +class Images::NestedComponent < ApplicationComponent + attr_reader :f, :image_fields + delegate :imageable_humanized_accepted_content_types, :imageable_max_file_size, to: :helpers + + def initialize(f, image_fields: :image) + @f = f + @image_fields = image_fields + end + + private + + def imageable + f.object + end + + def imageables_note + t "images.form.note", accepted_content_types: imageable_humanized_accepted_content_types, + max_file_size: imageable_max_file_size + end +end diff --git a/app/helpers/imageables_helper.rb b/app/helpers/imageables_helper.rb index b1734b4e2..c0e4c4893 100644 --- a/app/helpers/imageables_helper.rb +++ b/app/helpers/imageables_helper.rb @@ -14,9 +14,4 @@ module ImageablesHelper def imageable_humanized_accepted_content_types Setting.accepted_content_types_for("images").join(", ") end - - def imageables_note(_imageable) - t "images.form.note", accepted_content_types: imageable_humanized_accepted_content_types, - max_file_size: imageable_max_file_size - end end diff --git a/app/views/admin/poll/questions/answers/images/new.html.erb b/app/views/admin/poll/questions/answers/images/new.html.erb index d271f2ca6..fa373fbc1 100644 --- a/app/views/admin/poll/questions/answers/images/new.html.erb +++ b/app/views/admin/poll/questions/answers/images/new.html.erb @@ -5,7 +5,7 @@ <%= render "shared/errors", resource: @answer %>
- <%= render "images/nested_image", f: f, image_fields: :images %> + <%= render Images::NestedComponent.new(f, image_fields: :images) %>
<%= f.submit t("admin.questions.answers.images.save_image"), class: "button success" %> diff --git a/app/views/images/_nested_image.html.erb b/app/views/images/_nested_image.html.erb index 4dacbede9..c55ccaa56 100644 --- a/app/views/images/_nested_image.html.erb +++ b/app/views/images/_nested_image.html.erb @@ -1,25 +1 @@ -<% image_fields ||= :image %> -<% imageable = f.object %> - -<%= f.label image_fields, t("images.form.title") %> -

<%= imageables_note(imageable) %>

- -
- <%= f.fields_for image_fields do |image_builder| %> - <%= render Images::FieldsComponent.new(image_builder, imageable: imageable) %> - <% end %> -
- -<%= link_to_add_association t("images.form.add_new_image"), f, image_fields, - force_non_association_create: true, - partial: "images/image_fields", - id: "new_image_link", - class: "button upload-image - #{"hide" if image_fields == :image && imageable.image.present?}", - render_options: { - locals: { imageable: imageable } - }, - data: { - association_insertion_node: "#nested-image", - association_insertion_method: "append" - } %> +<%= render Images::NestedComponent.new(f) %> From 67c29a7c5f5df04cab658b94904c424c0e02a171 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 22:02:07 +0200 Subject: [PATCH 14/22] Remove duplication in max documents allowed code --- app/components/documents/nested_component.html.erb | 2 +- app/components/documents/nested_component.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/components/documents/nested_component.html.erb b/app/components/documents/nested_component.html.erb index acff7d0e3..bdad97aa3 100644 --- a/app/components/documents/nested_component.html.erb +++ b/app/components/documents/nested_component.html.erb @@ -2,7 +2,7 @@ <%= f.label :documents, t("documents.form.title") %>

<%= documentables_note %>

-
+
<%= f.fields_for :documents do |documents_builder| %> <%= render Documents::FieldsComponent.new(documents_builder) %> <% end %> diff --git a/app/components/documents/nested_component.rb b/app/components/documents/nested_component.rb index 14ddd857e..118cbfc5f 100644 --- a/app/components/documents/nested_component.rb +++ b/app/components/documents/nested_component.rb @@ -23,6 +23,6 @@ class Documents::NestedComponent < ApplicationComponent end def max_documents_allowed? - documentable.documents.count >= documentable.class.max_documents_allowed + documentable.documents.count >= max_documents_allowed end end From c9903f36cc9c3d0bd09a423d9d5c59a16fcc83bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 11 Jul 2021 22:03:32 +0200 Subject: [PATCH 15/22] Simplify imageable/documentable note method names --- app/components/documents/nested_component.html.erb | 2 +- app/components/documents/nested_component.rb | 2 +- app/components/images/nested_component.html.erb | 2 +- app/components/images/nested_component.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/components/documents/nested_component.html.erb b/app/components/documents/nested_component.html.erb index bdad97aa3..ec6a07d65 100644 --- a/app/components/documents/nested_component.html.erb +++ b/app/components/documents/nested_component.html.erb @@ -1,6 +1,6 @@
<%= f.label :documents, t("documents.form.title") %> -

<%= documentables_note %>

+

<%= note %>

<%= f.fields_for :documents do |documents_builder| %> diff --git a/app/components/documents/nested_component.rb b/app/components/documents/nested_component.rb index 118cbfc5f..a37587e81 100644 --- a/app/components/documents/nested_component.rb +++ b/app/components/documents/nested_component.rb @@ -16,7 +16,7 @@ class Documents::NestedComponent < ApplicationComponent documentable.class.max_documents_allowed end - def documentables_note + def note t "documents.form.note", max_documents_allowed: max_documents_allowed, accepted_content_types: documentable_humanized_accepted_content_types(documentable.class), max_file_size: max_file_size(documentable.class) diff --git a/app/components/images/nested_component.html.erb b/app/components/images/nested_component.html.erb index fc788c8bd..316c6474e 100644 --- a/app/components/images/nested_component.html.erb +++ b/app/components/images/nested_component.html.erb @@ -1,5 +1,5 @@ <%= f.label image_fields, t("images.form.title") %> -

<%= imageables_note %>

+

<%= note %>

<%= f.fields_for image_fields do |image_builder| %> diff --git a/app/components/images/nested_component.rb b/app/components/images/nested_component.rb index 04d4dc5d4..61087bcff 100644 --- a/app/components/images/nested_component.rb +++ b/app/components/images/nested_component.rb @@ -13,7 +13,7 @@ class Images::NestedComponent < ApplicationComponent f.object end - def imageables_note + def note t "images.form.note", accepted_content_types: imageable_humanized_accepted_content_types, max_file_size: imageable_max_file_size end From 394a94cbff842e62565625083d46da19c4a662fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 00:31:40 +0200 Subject: [PATCH 16/22] Fix invalid HTML in document/image fields labels These labels weren't associated with any fields, which is invalid HTML. We're using a legend inside a fieldset instead, which is the natural way to group form fields together. --- .../documents/nested_component.html.erb | 6 +-- .../images/nested_component.html.erb | 42 ++++++++++--------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/app/components/documents/nested_component.html.erb b/app/components/documents/nested_component.html.erb index ec6a07d65..743cd7be3 100644 --- a/app/components/documents/nested_component.html.erb +++ b/app/components/documents/nested_component.html.erb @@ -1,5 +1,5 @@ -
- <%= f.label :documents, t("documents.form.title") %> +
+ <%= t("documents.form.title") %>

<%= note %>

@@ -21,4 +21,4 @@
"> <%= sanitize(t("documents.max_documents_allowed_reached")) %>
-
+
diff --git a/app/components/images/nested_component.html.erb b/app/components/images/nested_component.html.erb index 316c6474e..0505ea4de 100644 --- a/app/components/images/nested_component.html.erb +++ b/app/components/images/nested_component.html.erb @@ -1,22 +1,24 @@ -<%= f.label image_fields, t("images.form.title") %> -

<%= note %>

+
+ <%= t("images.form.title") %> +

<%= note %>

-
- <%= f.fields_for image_fields do |image_builder| %> - <%= render Images::FieldsComponent.new(image_builder, imageable: imageable) %> - <% end %> -
+
+ <%= f.fields_for image_fields do |image_builder| %> + <%= render Images::FieldsComponent.new(image_builder, imageable: imageable) %> + <% end %> +
-<%= link_to_add_association t("images.form.add_new_image"), f, image_fields, - force_non_association_create: true, - partial: "images/image_fields", - id: "new_image_link", - class: "button upload-image - #{"hide" if image_fields == :image && imageable.image.present?}", - render_options: { - locals: { imageable: imageable } - }, - data: { - association_insertion_node: "#nested-image", - association_insertion_method: "append" - } %> + <%= link_to_add_association t("images.form.add_new_image"), f, image_fields, + force_non_association_create: true, + partial: "images/image_fields", + id: "new_image_link", + class: "button upload-image + #{"hide" if image_fields == :image && imageable.image.present?}", + render_options: { + locals: { imageable: imageable } + }, + data: { + association_insertion_node: "#nested-image", + association_insertion_method: "append" + } %> +
From 8cdee167f8d5a00d51d424db364e05a43242fdfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 01:15:40 +0200 Subject: [PATCH 17/22] Fix duplicate HTML ID in document fields Using `dom_id` means generating `new_document` as ID for new documents. Since there might be more than one new document in the form, that means duplicate IDs, which is invalid HTML. Even though this issue doesn't affect image fields (because we don't have many images on the same form), we're removing the ID there as well for consistency. --- app/components/documents/fields_component.html.erb | 2 +- app/components/images/fields_component.html.erb | 2 +- spec/shared/system/nested_documentable.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb index 4dc0c42ff..91d1a19d5 100644 --- a/app/components/documents/fields_component.html.erb +++ b/app/components/documents/fields_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= f.hidden_field :id %> <%= f.hidden_field :user_id, value: current_user.id %> <%= f.hidden_field :cached_attachment %> diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb index bff9555f1..00985abc0 100644 --- a/app/components/images/fields_component.html.erb +++ b/app/components/images/fields_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= f.hidden_field :id %> <%= f.hidden_field :user_id, value: current_user.id %> <%= f.hidden_field :cached_attachment %> diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index b6b1175a6..ed2eabbef 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -320,7 +320,7 @@ end def documentable_attach_new_file(path, success = true) click_link "Add new document" - document = all("#new_document").last + document = all(".document").last document_input = document.find("input[type=file]", visible: :hidden) attach_file(document_input[:id], path, make_visible: true) From acbd1b02396e3919daeb601f00d2ddda79ebe748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 03:19:34 +0200 Subject: [PATCH 18/22] Simplify code to reset attachment fields Since elements are created and destroyed, we don't have to do any kind of "reset" before destroying or creating them. We would have to do so if we were just hiding the elements in order to show them again later, but that's not the case. --- app/assets/javascripts/documentable.js | 5 ----- app/assets/javascripts/imageable.js | 9 --------- 2 files changed, 14 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 1a7cf33ff..53057ff79 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -134,11 +134,6 @@ "_method": "delete" }, complete: function() { - $(data.cachedAttachmentField).val(""); - $(data.addAttachmentLabel).show(); - App.Documentable.clearFilename(data); - App.Documentable.clearInputErrors(data); - App.Documentable.clearProgressBar(data); App.Documentable.unlockUploads(); $(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); $(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 5b784f475..62b3b9731 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -8,9 +8,6 @@ $("#nested-image").on("cocoon:after-remove", function() { $("#new_image_link").removeClass("hide"); }); - $("#nested-image").on("cocoon:before-insert", function() { - $(".js-image-attachment").closest(".image").remove(); - }); $("#nested-image").on("cocoon:after-insert", function(e, nested_image) { var input; $("#new_image_link").addClass("hide"); @@ -137,12 +134,6 @@ "_method": "delete" }, complete: function() { - $(data.cachedAttachmentField).val(""); - $(data.addAttachmentLabel).show(); - App.Imageable.clearFilename(data); - App.Imageable.clearInputErrors(data); - App.Imageable.clearProgressBar(data); - App.Imageable.clearPreview(data); $("#new_image_link").removeClass("hide"); $(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); $(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); From a7e2f1ae30655488c1c8e23b03a6e73e5be38116 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 03:34:59 +0200 Subject: [PATCH 19/22] Move file name before the destroy attachment link This way screen reader users will hear the name of the file before hearing about the link to destroy it. We were already displaying it this way visually by having the file name on the left and the destroy link on the right. Thanks to this change we can also simplify the code which dynamically changed the layout. --- app/assets/javascripts/documentable.js | 4 ---- app/assets/javascripts/imageable.js | 4 ---- app/assets/stylesheets/mixins/uploads.scss | 1 + app/components/documents/fields_component.html.erb | 7 +++---- app/components/images/fields_component.html.erb | 7 +++---- 5 files changed, 7 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 53057ff79..3a823dab7 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -55,8 +55,6 @@ App.Documentable.setFilename(data, data.result.filename); App.Documentable.clearInputErrors(data); $(data.addAttachmentLabel).hide(); - $(data.wrapper).find(".attachment-actions").removeClass("small-12").addClass("small-6 float-right"); - $(data.wrapper).find(".attachment-actions .action-remove").removeClass("small-3").addClass("small-12"); destroyAttachmentLink = $(data.result.destroy_link); $(data.destroyAttachmentLinkContainer).html(destroyAttachmentLink); $(destroyAttachmentLink).on("click", function(event) { @@ -135,8 +133,6 @@ }, complete: function() { App.Documentable.unlockUploads(); - $(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); - $(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); if ($(data.input).data("nested-document") === true) { $(data.wrapper).remove(); } else { diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 62b3b9731..86ef7346e 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -53,8 +53,6 @@ App.Imageable.setFilename(data, data.result.filename); App.Imageable.clearInputErrors(data); $(data.addAttachmentLabel).hide(); - $(data.wrapper).find(".attachment-actions").removeClass("small-12").addClass("small-6 float-right"); - $(data.wrapper).find(".attachment-actions .action-remove").removeClass("small-3").addClass("small-12"); App.Imageable.setPreview(data); destroyAttachmentLink = $(data.result.destroy_link); $(data.destroyAttachmentLinkContainer).html(destroyAttachmentLink); @@ -135,8 +133,6 @@ }, complete: function() { $("#new_image_link").removeClass("hide"); - $(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); - $(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); if ($(data.input).data("nested-image") === true) { $(data.wrapper).remove(); } else { diff --git a/app/assets/stylesheets/mixins/uploads.scss b/app/assets/stylesheets/mixins/uploads.scss index 1aa737553..d5c5936c6 100644 --- a/app/assets/stylesheets/mixins/uploads.scss +++ b/app/assets/stylesheets/mixins/uploads.scss @@ -41,6 +41,7 @@ } .file-name { + padding-left: 0; margin-top: 0; &:empty { diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb index 91d1a19d5..4ae94c81b 100644 --- a/app/components/documents/fields_component.html.erb +++ b/app/components/documents/fields_component.html.erb @@ -11,15 +11,14 @@
<%= file_field %>
+ +

<%= file_name %>

+
<%= destroy_link %>
-
-

<%= file_name %>

-
-
diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb index 00985abc0..b83251726 100644 --- a/app/components/images/fields_component.html.erb +++ b/app/components/images/fields_component.html.erb @@ -13,15 +13,14 @@
<%= file_field %>
+ +

<%= file_name %>

+
<%= destroy_link %>
-
-

<%= file_name %>

-
-
From 367d3f9d1499cd20f953bfdb6feea18096c0c98e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 04:14:19 +0200 Subject: [PATCH 20/22] Remove condition for non-nested case We don't use these input fields inside a non-nested context since commit 2993ef870. --- app/assets/javascripts/documentable.js | 7 +------ app/assets/javascripts/imageable.js | 7 +------ app/components/documents/fields_component.rb | 5 +---- app/components/images/fields_component.rb | 5 +---- 4 files changed, 4 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 3a823dab7..03d0a0f02 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -76,7 +76,6 @@ buildData: function(data, input) { var wrapper; wrapper = $(input).closest(".direct-upload"); - data.input = input; data.wrapper = wrapper; data.progressBar = $(wrapper).find(".progress-bar-placeholder"); data.errorContainer = $(wrapper).find(".attachment-errors"); @@ -133,11 +132,7 @@ }, complete: function() { App.Documentable.unlockUploads(); - if ($(data.input).data("nested-document") === true) { - $(data.wrapper).remove(); - } else { - $(data.wrapper).find("a.remove-cached-attachment").remove(); - } + $(data.wrapper).remove(); } }); }, diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 86ef7346e..75b68ed40 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -72,7 +72,6 @@ buildData: function(data, input) { var wrapper; wrapper = $(input).closest(".direct-upload"); - data.input = input; data.wrapper = wrapper; data.progressBar = $(wrapper).find(".progress-bar-placeholder"); data.preview = $(wrapper).find(".image-preview"); @@ -133,11 +132,7 @@ }, complete: function() { $("#new_image_link").removeClass("hide"); - if ($(data.input).data("nested-image") === true) { - $(data.wrapper).remove(); - } else { - $(data.wrapper).find("a.remove-cached-attachment").remove(); - } + $(data.wrapper).remove(); } }); }, diff --git a/app/components/documents/fields_component.rb b/app/components/documents/fields_component.rb index 4d503d912..3aa0126bf 100644 --- a/app/components/documents/fields_component.rb +++ b/app/components/documents/fields_component.rb @@ -39,10 +39,7 @@ class Documents::FieldsComponent < ApplicationComponent label_options: { class: "button hollow #{klass}" }, accept: accepted_content_types_extensions, class: "js-document-attachment", - data: { - url: direct_upload_path, - nested_document: true - } + data: { url: direct_upload_path } end def direct_upload_path diff --git a/app/components/images/fields_component.rb b/app/components/images/fields_component.rb index 0cc91d28c..9b6ceb80d 100644 --- a/app/components/images/fields_component.rb +++ b/app/components/images/fields_component.rb @@ -40,10 +40,7 @@ class Images::FieldsComponent < ApplicationComponent label_options: { class: "button hollow #{klass}" }, accept: accepted_content_types_extensions, class: "js-image-attachment", - data: { - url: direct_upload_path, - nested_image: true - } + data: { url: direct_upload_path } end def direct_upload_path From 6da8eeac6fee9bee5523bfca8620cbc7c00765b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 04:51:05 +0200 Subject: [PATCH 21/22] Simplify code to delete a cached attachment We were already defining the links with data-remote and data-method, so instead of manually doing an AJAX request we can rely on Rails to perform the request and then handle the `ajax:complete` event. --- app/assets/javascripts/documentable.js | 34 ++++---------------------- app/assets/javascripts/imageable.js | 33 ++++--------------------- 2 files changed, 10 insertions(+), 57 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 03d0a0f02..2d7160f00 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -17,11 +17,11 @@ App.Documentable.lockUploads(); } }); + App.Documentable.initializeRemoveCachedDocumentLinks(); }, initializeDirectUploadInput: function(input) { var inputData; inputData = this.buildData([], input); - this.initializeRemoveCachedDocumentLink(input, inputData); $(input).fileupload({ paramName: "attachment", formData: null, @@ -57,11 +57,6 @@ $(data.addAttachmentLabel).hide(); destroyAttachmentLink = $(data.result.destroy_link); $(data.destroyAttachmentLinkContainer).html(destroyAttachmentLink); - $(destroyAttachmentLink).on("click", function(event) { - event.preventDefault(); - event.stopPropagation(); - App.Documentable.doDeleteCachedAttachmentRequest(this.href, data); - }); if (input.lockUpload) { App.Documentable.showNotice(); } @@ -76,7 +71,6 @@ buildData: function(data, input) { var wrapper; wrapper = $(input).closest(".direct-upload"); - data.wrapper = wrapper; data.progressBar = $(wrapper).find(".progress-bar-placeholder"); data.errorContainer = $(wrapper).find(".attachment-errors"); data.fileNameContainer = $(wrapper).find("p.file-name"); @@ -122,28 +116,10 @@ showNotice: function() { $("#max-documents-notice").removeClass("hide"); }, - doDeleteCachedAttachmentRequest: function(url, data) { - $.ajax({ - type: "POST", - url: url, - dataType: "json", - data: { - "_method": "delete" - }, - complete: function() { - App.Documentable.unlockUploads(); - $(data.wrapper).remove(); - } - }); - }, - initializeRemoveCachedDocumentLink: function(input, data) { - var remove_document_link, wrapper; - wrapper = $(input).closest(".direct-upload"); - remove_document_link = $(wrapper).find("a.remove-cached-attachment"); - $(remove_document_link).on("click", function(e) { - e.preventDefault(); - e.stopPropagation(); - App.Documentable.doDeleteCachedAttachmentRequest(this.href, data); + initializeRemoveCachedDocumentLinks: function() { + $("#nested-documents").on("ajax:complete", "a.remove-cached-attachment", function() { + App.Documentable.unlockUploads(); + $(this).closest(".direct-upload").remove(); }); }, removeDocument: function(id) { diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 75b68ed40..7b24b097b 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -14,11 +14,11 @@ input = $(nested_image).find(".js-image-attachment"); App.Imageable.initializeDirectUploadInput(input); }); + App.Imageable.initializeRemoveCachedImageLinks(); }, initializeDirectUploadInput: function(input) { var inputData; inputData = this.buildData([], input); - this.initializeRemoveCachedImageLink(input, inputData); $(input).fileupload({ paramName: "attachment", formData: null, @@ -56,11 +56,6 @@ App.Imageable.setPreview(data); destroyAttachmentLink = $(data.result.destroy_link); $(data.destroyAttachmentLinkContainer).html(destroyAttachmentLink); - $(destroyAttachmentLink).on("click", function(event) { - event.preventDefault(); - event.stopPropagation(); - App.Imageable.doDeleteCachedAttachmentRequest(this.href, data); - }); }, progress: function(e, data) { var progress; @@ -122,28 +117,10 @@ data.preview = $(data.wrapper).find(".image-preview"); } }, - doDeleteCachedAttachmentRequest: function(url, data) { - $.ajax({ - type: "POST", - url: url, - dataType: "json", - data: { - "_method": "delete" - }, - complete: function() { - $("#new_image_link").removeClass("hide"); - $(data.wrapper).remove(); - } - }); - }, - initializeRemoveCachedImageLink: function(input, data) { - var remove_image_link, wrapper; - wrapper = $(input).closest(".direct-upload"); - remove_image_link = $(wrapper).find("a.remove-cached-attachment"); - $(remove_image_link).on("click", function(e) { - e.preventDefault(); - e.stopPropagation(); - App.Imageable.doDeleteCachedAttachmentRequest(this.href, data); + initializeRemoveCachedImageLinks: function() { + $("#nested-image").on("ajax:complete", "a.remove-cached-attachment", function() { + $("#new_image_link").removeClass("hide"); + $(this).closest(".direct-upload").remove(); }); }, removeImage: function(id) { From cc6f9391fc47844ed99dfac729aabfa7b6b20536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 12 Jul 2021 05:07:39 +0200 Subject: [PATCH 22/22] Fix attaching files using the keyboard We were hiding the file input and styling the label as a button instead. Since clicking on a label has the same effect as clicking on the input, the input worked properly for mouse and touch screen users. However, hiding the input makes it inaccessible for keyboard users, since labels don't get keyboard focus, but inputs do. So we must not hide the input but make it invisible instead. But we still need to hide the input (alongside the label) after a file has been attached. We could add some extra JavaScript to hide the input when we hide the label. Since the JavaScript is already quite complex and my first few attempts at changing it failed, I've opted to assume that the input (and its label) must be hidden whenever there's already a file name, and implement that rule with CSS. Note we're using the `:focus-within` pseudoclass to style a label when focus is on the input. This rule (at the time of writing) is only supported by 93.5% of the browsers. Keyboard users without a screen reader and using the other 6.5% of the browsers will still be able to focus on the field but might not notice the field has received focus. Since the percentage of affected users will decrease over time and until now 100% of keyboard users were completely unable to focus on these fields, for now we think this is a good-enough solution. --- app/assets/javascripts/documentable.js | 2 -- app/assets/javascripts/imageable.js | 2 -- app/assets/stylesheets/mixins/uploads.scss | 11 ++++++++++- .../documents/fields_component.html.erb | 4 ++-- app/components/images/fields_component.html.erb | 4 ++-- spec/shared/system/nested_documentable.rb | 17 +++-------------- spec/shared/system/nested_imageable.rb | 17 +++-------------- spec/system/admin/widgets/cards_spec.rb | 7 ++----- 8 files changed, 22 insertions(+), 42 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 2d7160f00..7aa9e1b29 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -45,7 +45,6 @@ App.Documentable.setInputErrors(data); $(data.destroyAttachmentLinkContainer).find("a.delete:not(.remove-nested)").remove(); $(data.addAttachmentLabel).addClass("error"); - $(data.addAttachmentLabel).show(); }, done: function(e, data) { var destroyAttachmentLink; @@ -54,7 +53,6 @@ App.Documentable.setProgressBar(data, "complete"); App.Documentable.setFilename(data, data.result.filename); App.Documentable.clearInputErrors(data); - $(data.addAttachmentLabel).hide(); destroyAttachmentLink = $(data.result.destroy_link); $(data.destroyAttachmentLinkContainer).html(destroyAttachmentLink); if (input.lockUpload) { diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 7b24b097b..c2ee3e882 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -43,7 +43,6 @@ App.Imageable.clearPreview(data); $(data.destroyAttachmentLinkContainer).find("a.delete:not(.remove-nested)").remove(); $(data.addAttachmentLabel).addClass("error"); - $(data.addAttachmentLabel).show(); }, done: function(e, data) { var destroyAttachmentLink; @@ -52,7 +51,6 @@ App.Imageable.setProgressBar(data, "complete"); App.Imageable.setFilename(data, data.result.filename); App.Imageable.clearInputErrors(data); - $(data.addAttachmentLabel).hide(); App.Imageable.setPreview(data); destroyAttachmentLink = $(data.result.destroy_link); $(data.destroyAttachmentLinkContainer).html(destroyAttachmentLink); diff --git a/app/assets/stylesheets/mixins/uploads.scss b/app/assets/stylesheets/mixins/uploads.scss index d5c5936c6..570b65326 100644 --- a/app/assets/stylesheets/mixins/uploads.scss +++ b/app/assets/stylesheets/mixins/uploads.scss @@ -20,13 +20,17 @@ p { margin-bottom: 0; } + + &:focus-within label { + outline: $outline-focus; + } } .attachment-errors { > .js-image-attachment, > .js-document-attachment { - display: none; + @include element-invisible; ~ .error { display: inline-block; @@ -47,6 +51,11 @@ &:empty { display: none; } + + &:not(:empty) + .document-attachment, + &:not(:empty) + .image-attachment { + display: none; + } } .loading-bar { diff --git a/app/components/documents/fields_component.html.erb b/app/components/documents/fields_component.html.erb index 4ae94c81b..5822f89ba 100644 --- a/app/components/documents/fields_component.html.erb +++ b/app/components/documents/fields_component.html.erb @@ -8,12 +8,12 @@
+

<%= file_name %>

+
<%= file_field %>
-

<%= file_name %>

-
<%= destroy_link %>
diff --git a/app/components/images/fields_component.html.erb b/app/components/images/fields_component.html.erb index b83251726..49d8f1fd8 100644 --- a/app/components/images/fields_component.html.erb +++ b/app/components/images/fields_component.html.erb @@ -10,12 +10,12 @@ <%= render_image(image, :thumb, false) if image.attachment.exists? %>
+

<%= file_name %>

+
<%= file_field %>
-

<%= file_name %>

-
<%= destroy_link %>
diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index ed2eabbef..315eda144 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -77,12 +77,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na click_link "Add new document" within "#nested-documents" do - document = find(".document input[type=file]", visible: :hidden) - attach_file( - document[:id], - Rails.root.join("spec/fixtures/files/empty.pdf"), - make_visible: true - ) + attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") expect(page).to have_css ".loading-bar.complete" end @@ -109,12 +104,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na within "#nested-documents" do input = find("input[name$='[title]']") fill_in input[:id], with: "My Title" - document_input = find("input[type=file]", visible: :hidden) - attach_file( - document_input[:id], - Rails.root.join("spec/fixtures/files/empty.pdf"), - make_visible: true - ) + attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") expect(page).to have_css ".loading-bar.complete" end @@ -321,8 +311,7 @@ def documentable_attach_new_file(path, success = true) click_link "Add new document" document = all(".document").last - document_input = document.find("input[type=file]", visible: :hidden) - attach_file(document_input[:id], path, make_visible: true) + attach_file "Choose document", path within document do if success diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index eddc3bab2..1560db273 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -38,12 +38,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) click_link "Add image" - image_input = find(".image").find("input[type=file]", visible: :hidden) - attach_file( - image_input[:id], - Rails.root.join("spec/fixtures/files/clippy.jpg"), - make_visible: true - ) + attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") expect(page).to have_selector ".file-name", text: "clippy.jpg" end @@ -67,12 +62,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p click_link "Add image" input_title = find(".image input[name$='[title]']") fill_in input_title[:id], with: "Title" - image_input = find(".image").find("input[type=file]", visible: :hidden) - attach_file( - image_input[:id], - Rails.root.join("spec/fixtures/files/clippy.jpg"), - make_visible: true - ) + attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") if has_many_images expect(find("input[id$='_title']").value).to eq "Title" @@ -271,8 +261,7 @@ def imageable_attach_new_file(_imageable_factory_name, path, success = true) click_link "Add image" within "#nested-image" do image = find(".image") - image_input = image.find("input[type=file]", visible: :hidden) - attach_file(image_input[:id], path, make_visible: true) + attach_file "Choose image", path within image do if success expect(page).to have_css(".loading-bar.complete") diff --git a/spec/system/admin/widgets/cards_spec.rb b/spec/system/admin/widgets/cards_spec.rb index dac6a03a7..9258e12e5 100644 --- a/spec/system/admin/widgets/cards_spec.rb +++ b/spec/system/admin/widgets/cards_spec.rb @@ -252,11 +252,8 @@ describe "Cards", :admin do def attach_image_to_card click_link "Add image" - image_input = all(".image").last.find("input[type=file]", visible: false) - attach_file( - image_input[:id], - Rails.root.join("spec/fixtures/files/clippy.jpg"), - make_visible: true) + attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + expect(page).to have_field("widget_card_image_attributes_title", with: "clippy.jpg") end end