Add Unique HTML IDs to .dropzones, #fileinputs #8927

noi5e · 2021-01-03T08:53:57Z

Adds unique HTML IDs to .dropzones and #fileinputs in comment forms.

A: currently: .dropzone (with no ID) wrapping #fileinput (which is used twice in the same comment form, or multiple times per page)
- after merging: #dropzone-small-edit-22
- after merging: #fileinput-button-edit-22
- after merging: #dropzone-reply-22 for reply forms, #dropzone-main for base comment form in notes, questions, and wikis
B: currently: .dropzone (with no ID) wrapping the form's entire face, same class as A
- after merging: #dropzone-large-reply-20 in reply forms and #dropzone-large-main in main forms
C: currently: second #fileinput here (distinct from A, but with the same ID)
- this is now #fileinput-choose-one-reply-1234 or #fileinput-choose-one-main

As usual, had to rewrite some of the old system tests that relied on the old classes and IDs.

There's probably more tests to rewrite before this is ready-to-merge, thought it would be faster/easier to see in CI rather than run tests locally.

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

gitpod-io · 2021-01-03T08:54:01Z

codecov · 2021-01-03T09:00:03Z

Codecov Report

❗ No coverage uploaded for pull request base (main@9c409ec). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #8927   +/-   ##
=======================================
  Coverage        ?   81.89%           
=======================================
  Files           ?      100           
  Lines           ?     5948           
  Branches        ?        0           
=======================================
  Hits            ?     4871           
  Misses          ?     1077           
  Partials        ?        0

jywarren

This looks amazing @noi5e -- i think it's pretty much good to merge unless you want to do more; my questions below are clarifications and "just checking" so i defer to you on a final call on any of them. If it's ready to merge just note that in a comment and we can do it!

Fantastic work here!!! 👍

jywarren · 2021-01-05T21:33:57Z

app/views/comments/_edit.html.erb

@@ -8,8 +8,8 @@
      #imagebar {width:100%;}
    </style>

-
-    <%= render :partial => "editor/toolbar" %>
+    <%# toolbar needs location & comment_id to make unique element IDs%>


here you could use an HTML comment perhaps?  just to simplify a little?

jywarren · 2021-01-05T21:35:23Z

app/views/comments/_form.html.erb

-              <label for="fileinput">
-                <input id="fileinput" type="file" name="image[photo]" style="display:none;" />
+              <label for="fileinput-choose-one<%= dropzone_large_id %>">
+                <input id="fileinput-choose-one<%= dropzone_large_id %>" type="file" name="image[photo]" style="display:none;" />


Will this change styling at all? Noting that sometimes adding unique classnames instead of ids can be better since elements are allowed to have more than one class but only one ID. I tend to use classes instead of IDs for that reason, but it's just a convention or habit, so no big deal here, just curious!

Oh, that's a really good point about styling. I did a simple search in VSCode for #fileinput and input#fileinput but it didn't come up with anything in /assets/stylesheets, so I think this is good. It looks as normal when I load it up in development too.

That's also a good point about classes vs. IDs. Since my ultimate goal is to be able to use e.target to set $D.selected upon file upload, I thought an ID made the most sense here... Good reminder though that I can use classes in other situations.

I was aware of the one ID per page rule. dropzone_large_id includes the comment ID, so it's going to resolve to a unique ID every time this form is rendered (#fileinput-reply-large-22 or something like that)... Which means that there shouldn't be any conflicting IDs here, at least throughout comments-list!

jywarren · 2021-01-05T21:36:10Z

app/views/notes/_comment.html.erb

@@ -166,7 +166,16 @@
        <div id="comment-<%= comment.cid %>-reply-section">
          <% if current_user %>
            <div class="inline" id="question-comment-form">
-              <%= render :partial => "comments/form", :locals => { title: "Reply to this comment", reply_to: comment.cid, comment: false, placeholder: "Help the author refine their post, or point them at other helpful information on the site. Mention users by @username to notify them of this thread by email", url1: '/conduct', author: current_user.title, is_new_contributor:current_user.is_new_contributor? } %>
+              <%= render :partial => "comments/form", :locals => { 


so much more readable, thank you!

jywarren · 2021-01-05T21:38:05Z

app/views/comments/_form.html.erb


      <% if local_assigns[:reply_to].present? %>
        <%= hidden_field_tag :reply_to, local_assigns[:reply_to] %>
      <% end %>

-      <div class="form-group dropzone">
-
+      <%# most comment forms have two dropzones: 1) small button, 2) large form that covers textarea %>


There's a case possibly to be made that logic doesn't belong in a template, but i see why this may be an exception. But just to explore the possibility, would it be possible to wrap this logic in a function and do it in a single line in the template, with the logic tucked into the application_helper.rb (helpers is where template-specific logic often goes, by convention)?

But, i'm not feeling strongly either way, there are pros and cons - it might be more readable in pure Ruby in the helper, but it's also more work to go and find it...

Thank you! This is great feedback, and helps me learn more about Rails conventions (which I'm going to be learning for a while). I went ahead and put the logic in application_helper.rb, just for the sake of developing good habits.

codeclimate · 2021-01-05T23:39:19Z

app/helpers/application_helper.rb

+  # used in views/comments/_form.html.erb
+  def get_large_dropzone_id(location, reply_to)
+    case location
+      when :main


Indent when as deep as end.

codeclimate · 2021-01-05T23:39:19Z

app/helpers/application_helper.rb

+    case location
+      when :main
+        '-main'
+      when :reply


Indent when as deep as end.

codeclimate · 2021-01-05T23:39:19Z

app/helpers/application_helper.rb

+        '-main'
+      when :reply
+        '-reply-' + reply_to.to_s
+      when :responses


Indent when as deep as end.

noi5e · 2021-01-05T23:39:23Z

I just pushed the commits, so this one's ready to merge!

jywarren · 2021-01-05T23:41:17Z

Awesome would you like to take code climates suggestions or just resolve them? Thanks!!!

codeclimate · 2021-01-05T23:45:48Z

Code Climate has analyzed commit 12726f4 and detected 0 issues on this pull request.

View more on Code Climate.

noi5e · 2021-01-06T00:09:33Z

Just pushed the commits, it's ready (again) now!

jywarren · 2021-01-06T01:26:30Z

Hooray 🎉👏👏

* generate unique HTML IDs for image upload .dropzones & inputs * update system tests referring to #fileinput * move get_dropzone_id logic to application_helper.rb publiclab#8926 * change comments to HTML comments * fix indents to pass linter

noi5e added 2 commits January 2, 2021 19:14

generate unique HTML IDs for image upload .dropzones & inputs

bc302a0

update system tests referring to #fileinput

37e4ed2

noi5e added bug the issue is regarding one of our programs which faces problems when a certain task is executed testing issues are usually for adding unit tests, integration tests or any other tests for a feature outreachy labels Jan 3, 2021

noi5e added this to the [Outreachy] Comment Editor Winter 2020 milestone Jan 3, 2021

noi5e requested review from jywarren, sagarpreet-chadha and cesswairimu January 3, 2021 08:53

noi5e requested review from a team as code owners January 3, 2021 08:53

noi5e mentioned this pull request Jan 3, 2021

OLD CLOSED Comment Editor Overhaul Issue #8775

Closed

noi5e mentioned this pull request Jan 5, 2021

[Discussion] Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues #8926

Closed

2 tasks

jywarren approved these changes Jan 5, 2021

View reviewed changes

noi5e added 2 commits January 5, 2021 15:37

move get_dropzone_id logic to application_helper.rb publiclab#8926

9a71180

change comments to HTML comments

43a66f4

codeclimate bot reviewed Jan 5, 2021

View reviewed changes

fix indents to pass linter

12726f4

noi5e mentioned this pull request Jan 6, 2021

Fix Image Drag & Drop Cross-Wiring in Edit Comment Form #8897

Merged

jywarren merged commit 0f770f2 into publiclab:main Jan 6, 2021

noi5e deleted the feature/add-unique-ids-to-dropzones branch January 6, 2021 03:41

noi5e mentioned this pull request Jan 23, 2021

Comment Editor Overhaul Project [Outreachy] #9069

Open

noi5e changed the title ~~Feature: Add Unique HTML IDs to .dropzones, #fileinputs~~ Add Unique HTML IDs to .dropzones, #fileinputs Jan 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unique HTML IDs to .dropzones, #fileinputs #8927

Add Unique HTML IDs to .dropzones, #fileinputs #8927

noi5e commented Jan 3, 2021 •

edited

Loading

gitpod-io bot commented Jan 3, 2021 •

edited

Loading

codecov bot commented Jan 3, 2021 •

edited

Loading

jywarren left a comment

jywarren Jan 5, 2021

jywarren Jan 5, 2021

noi5e Jan 5, 2021

jywarren Jan 5, 2021

jywarren Jan 5, 2021

noi5e Jan 5, 2021

codeclimate bot Jan 5, 2021

codeclimate bot Jan 5, 2021

codeclimate bot Jan 5, 2021

noi5e commented Jan 5, 2021

jywarren commented Jan 5, 2021

codeclimate bot commented Jan 5, 2021

noi5e commented Jan 6, 2021

jywarren commented Jan 6, 2021

Add Unique HTML IDs to .dropzones, #fileinputs #8927

Add Unique HTML IDs to .dropzones, #fileinputs #8927

Conversation

noi5e commented Jan 3, 2021 • edited Loading

gitpod-io bot commented Jan 3, 2021 • edited Loading

codecov bot commented Jan 3, 2021 • edited Loading

Codecov Report

jywarren left a comment

Choose a reason for hiding this comment

jywarren Jan 5, 2021

Choose a reason for hiding this comment

jywarren Jan 5, 2021

Choose a reason for hiding this comment

noi5e Jan 5, 2021

Choose a reason for hiding this comment

jywarren Jan 5, 2021

Choose a reason for hiding this comment

jywarren Jan 5, 2021

Choose a reason for hiding this comment

noi5e Jan 5, 2021

Choose a reason for hiding this comment

codeclimate bot Jan 5, 2021

Choose a reason for hiding this comment

codeclimate bot Jan 5, 2021

Choose a reason for hiding this comment

codeclimate bot Jan 5, 2021

Choose a reason for hiding this comment

noi5e commented Jan 5, 2021

jywarren commented Jan 5, 2021

codeclimate bot commented Jan 5, 2021

noi5e commented Jan 6, 2021

jywarren commented Jan 6, 2021

noi5e commented Jan 3, 2021 •

edited

Loading

gitpod-io bot commented Jan 3, 2021 •

edited

Loading

codecov bot commented Jan 3, 2021 •

edited

Loading