Add `cms:page_file_link` tag to render page files #799

glebm · 2018-02-27T21:13:47Z

Examples:

{{ cms:page_file_link header }}
{{ cms:page_file_link attachments, filename: "cat.jpg", as: image }}

Dragging page-level files onto the textarea generates these tags.

See #796 for more information.
Fixes #796

GBH · 2018-02-27T21:50:33Z

app/views/comfy/admin/cms/fragments/_form_fragment_attachments.html.haml

+      = link_to truncated_filename, attachment,
+                data: {toggle: "page-file-popover", content: thumb},
+                class: "btn btn-light text-truncate", target: "_blank",
+                ondragstart: "event.dataTransfer.setData('text/plain', '#{cms_page_file_link_tag}');"


Don't think we can do inline JS like that if CSP is enabled. Let's move that into data attribute. Also how would one even know that this is something draggable?

Done. I didn't write in CoffeeScript, but it's only a few line so perhaps you could rewrite it if you really want it to be CoffeeScript?

Also how would one even know that this is something draggable?

True that it's not very discoverable, but it does have cursor: pointer on hover, and I've now made a thumbnail the drag image for images.

GBH · 2018-02-27T21:51:15Z

app/views/comfy/admin/cms/fragments/_form_fragment_attachments.html.haml

+          (", filename: \"#{filename}\"" if multiple),
+          (", as: image" if attachment.image?),
+          " }}".html_safe
+        ].compact.join


Kinda ugly. Let's move that into admin view helper.

GBH · 2018-02-27T21:51:34Z

lib/comfortable_mexican_sofa/content/tags/file.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true

+require "comfortable_mexican_sofa/content/tags/file_content.rb"


require_relative ?

GBH · 2018-02-27T21:53:25Z

lib/comfortable_mexican_sofa/content/tags/file_link.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true

+require "comfortable_mexican_sofa/content/tags/file_content.rb"


require_relative

GBH · 2018-02-27T21:53:56Z

lib/comfortable_mexican_sofa/content/tags/page_file_link.rb

@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+
+require "comfortable_mexican_sofa/content/tags/file_content.rb"


require_relative

GBH · 2018-02-27T21:55:07Z

lib/comfortable_mexican_sofa/content/tags/page_file_link.rb

+                fragment.attachments.first
+              else
+                fragment.attachments.detect { |a| a.filename.to_s == filename }
+              end


Don't like massive offsets like these. Should be:

foo ||= if bar?

I thought rubocop would catch this

glebm · 2018-02-28T00:16:56Z

I'll also move page file popovers into the same file

glebm · 2018-02-28T00:27:04Z

All done

glebm · 2018-02-28T05:14:43Z

There is a small glitch with the popover re-appearing at the end of the drop on Firefox, the popover re-appears:

Looking into it

glebm · 2018-02-28T06:35:25Z

Firefox glitch fixed

glebm · 2018-02-28T07:07:35Z

Files can now also be dragged out of the global files modal

GBH · 2018-02-28T18:34:41Z

For the JS drag-and-drop feature, do you mind writing a system test please? Capybara can do .drag_to thing. I only have couple of tests there, but anything involving JS interactions should have a system test.

glebm · 2018-02-28T18:47:40Z

OK, I'll try. Everything else looks good to you?

glebm · 2018-02-28T20:14:22Z

I wrote a system test as far as I could take it, because drag_to does not seem to work! Tried with the latest chromedriver too.

glebm · 2018-02-28T20:50:38Z

I've now written tests for both global files and page files drag'n'drop but they don't work and so are currently being skipped.

I've tried for a while to fix them but I couldn't. I won't look into this further.

GBH · 2018-02-28T21:20:43Z

I'm going to take a look at the drag_to issue. Writing capybara tests is like pulling teeth even when everything works as it should.

Rest of PR looks fine, however tests right now are failing. Also, don't introduce new fixture items just for one test. Either mutate existing or spin up new records manually. System tests run in transactions, so it's safe to do.

About coffeescript, I am not against converting to regular JS. One less dependency to worry about. I'm just not sure what the cool kids do these days.

glebm · 2018-02-28T21:30:27Z

Cool kids these days use TypeScript

glebm · 2018-02-28T21:35:43Z

I've removed the extra fixtures.

I'm going to take a look at the drag_to issue.

Start with the page files test as it's simpler (no iframes).

glebm · 2018-02-28T22:04:12Z

I've made a drag'n'drop video for the release notes (used Peek to record):

Examples: {{ cms:page_file_link header }} {{ cms:page_file_link attachments, filename: "cat.jpg", as: image }} Dragging page-level files onto the textarea or CodeMirror generates these tags. Site files can now also be dragged from the files modal onto the textarea or CodeMirror.

GBH · 2018-03-04T04:19:53Z

I'm in process of merging. Running locally and it's great. Dragging files from file pop-up to places is awesome. Gonna leave comments, but just as notes.

GBH · 2018-03-04T04:22:06Z

test/test_helper.rb

+  /usr/lib/chromium-browser/chromedriver
+  /usr/local/bin/chromedriver
+].find { |path| File.executable?(path) }
+


Had to remove this. Broke it for me as it wouldn't find my chromedriver that lives in a different bin directory (but still on my $PATH). There has to be a documentation on Rails Guides for installing all the crap. Guides only have a bit about testing, but nothing about how to get it all running in the first place.

GBH · 2018-03-04T05:39:57Z

lib/comfortable_mexican_sofa/content/tags/file_content.rb

+# frozen_string_literal: true
+
+# A mixin for tags that returns the file as their content.
+module ComfortableMexicanSofa::Content::Tag::FileContent


moving this file into /mixins as it's not a tag

GBH · 2018-03-04T05:43:56Z

app/views/comfy/admin/cms/files/_file.html.haml

@@ -1,4 +1,7 @@
 %li{data: {id: file.id}}
+  :ruby
+    file_tag = "{{ cms:file_link #{file.id} }}"


I think I want the same dealio to generate as: image for image attachments. Will expand that helper (or make new one) to do this.

GBH · 2018-03-04T06:32:04Z

test/system/files_frontend_test.rb

+    @site = comfy_cms_sites(:default)
+  end
+
+  def test_site_file_drag_and_drop


seems like we can't test this due to chromedriver limitation. leaving skip but removing code.

GBH · 2018-03-04T07:00:51Z

test/system/files_frontend_test.rb

+
+    # Dragging the link should generate CMS markup.
+    skip "Drag'n'drop does not work for some reason"
+    find_link("image.jpg").drag_to(content_field)


Can't figure out this one either. By all accounts it should work. Dragging other links should work too (to eliminate js fuckery) but it does not. Maybe warrants filing issue with Capybara.

GBH · 2018-03-04T21:28:50Z

@glebm can you double check if everything works in Firefox? On my VM (Firefox 50 though) none of the draggable stuff works as in Chrome. Dragging from iframe doesn't work at all and dragging from page files dumps link's url and not the tag's string.

I'm trying to upgrade to latest Firefox here. If latest version works then no worries.

glebm · 2018-03-04T21:33:28Z

It works for me perfectly in Firefox 58 on Ubuntu 16.04. Firefox 50 is from June 2016. I'll double check with the current comfy master.

GBH · 2018-03-04T22:43:51Z

Got Firefox 58 on LinuxMint (Ubuntu). Page file link drag works perfectly. One thing that still doesn't work is that drag from the iframe. In Chrome iframe goes away and it's possible to drag into form input. In Firefox iframe sticks around.

glebm · 2018-03-04T23:39:23Z

@GBH Reproduced, fixed in #802

glebm force-pushed the page-file branch 3 times, most recently from de13762 to 092e8a9 Compare February 27, 2018 21:17

GBH requested changes Feb 27, 2018

View reviewed changes

glebm force-pushed the page-file branch 2 times, most recently from 9e02fce to 897a83a Compare February 28, 2018 00:32

glebm force-pushed the page-file branch from dc251e5 to 8f3da4c Compare February 28, 2018 06:38

glebm force-pushed the page-file branch 2 times, most recently from 5a9814e to 0e89b01 Compare February 28, 2018 07:17

glebm force-pushed the page-file branch 2 times, most recently from ae134b9 to 0341c68 Compare February 28, 2018 21:18

glebm force-pushed the page-file branch from 4e422bc to d1707f0 Compare March 1, 2018 08:56

glebm mentioned this pull request Mar 1, 2018

Clean up files modal JavaScript #802

Merged

GBH reviewed Mar 4, 2018

View reviewed changes

GBH merged commit d1707f0 into comfy:master Mar 4, 2018

GBH reviewed Mar 4, 2018

View reviewed changes

glebm deleted the page-file branch March 5, 2018 00:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `cms:page_file_link` tag to render page files #799

Add `cms:page_file_link` tag to render page files #799

glebm commented Feb 27, 2018

GBH Feb 27, 2018

glebm Feb 28, 2018 •

edited

Loading

GBH Feb 27, 2018

glebm Feb 28, 2018

GBH Feb 27, 2018

glebm Feb 28, 2018

GBH Feb 27, 2018

GBH Feb 27, 2018

glebm Feb 28, 2018

GBH Feb 27, 2018

glebm Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018 •

edited

Loading

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

GBH commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

GBH commented Feb 28, 2018 •

edited

Loading

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018 •

edited

Loading

GBH commented Mar 4, 2018

GBH Mar 4, 2018

GBH Mar 4, 2018

GBH Mar 4, 2018

GBH Mar 4, 2018

GBH Mar 4, 2018

GBH commented Mar 4, 2018

glebm commented Mar 4, 2018 •

edited

Loading

GBH commented Mar 4, 2018

glebm commented Mar 4, 2018

		@@ -1,5 +1,7 @@
		# frozen_string_literal: true

		require "comfortable_mexican_sofa/content/tags/file_content.rb"

		@@ -0,0 +1,80 @@
		# frozen_string_literal: true

		require "comfortable_mexican_sofa/content/tags/file_content.rb"

Add cms:page_file_link tag to render page files #799

Add cms:page_file_link tag to render page files #799

Conversation

glebm commented Feb 27, 2018

Choose a reason for hiding this comment

glebm Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018 • edited Loading

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

GBH commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

GBH commented Feb 28, 2018 • edited Loading

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018

glebm commented Feb 28, 2018 • edited Loading

GBH commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GBH commented Mar 4, 2018

glebm commented Mar 4, 2018 • edited Loading

GBH commented Mar 4, 2018

glebm commented Mar 4, 2018

Add `cms:page_file_link` tag to render page files #799

Add `cms:page_file_link` tag to render page files #799

glebm Feb 28, 2018 •

edited

Loading

glebm commented Feb 28, 2018 •

edited

Loading

GBH commented Feb 28, 2018 •

edited

Loading

glebm commented Feb 28, 2018 •

edited

Loading

glebm commented Mar 4, 2018 •

edited

Loading