Problem/Motivation

The Image Insane sandbox module implements a really nice UI for uploading and changing images.

See the demo video: http://vimeo.com/112753990

Twitter conversation: https://twitter.com/lewisnyman/status/563621457792872448

Proposed resolution

See #12:

Remaining tasks

Discuss
Implement

User interface changes

A better inline image uploader

API changes

None

Files: 

Comments

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

I tried to find the corresponding module, but can't find it. Can you find it, Lewis? Or do you perhaps know the creator of that video?

LewisNyman’s picture

Yep it's still a sandbox - https://www.drupal.org/sandbox/avitslv/2413017

Ah there's no code pushed. I'll ask the maintainer

Manjit.Singh’s picture

Amazing module!! it would become so easy to change images if it published :)

chris_h’s picture

This is really nice. Would be good to get a similar function in for edit forms - for multivalue uploads especially drag and drop is a massive timesaver. https://www.drupal.org/project/dragndrop_upload is pretty good on D7

avitslv’s picture

I have pushed the code to the sandbox project https://www.drupal.org/sandbox/avitslv/2413017 :)
And I have created a PA issue here https://www.drupal.org/node/2451563

For now it works with D7 and using only image formatter.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

yoroy’s picture

Issue tags: +Needs design

Would be nice to see this in a prototype.

Wim Leers’s picture

#8++

Gábor Hojtsy’s picture

Issue tags: +Media Initiative
samuel.mortenson’s picture

I had no idea this issue existed, but I pushed up an experimental module to Github recently that has a similar feature set for Drupal 8/Quickedit: https://github.com/mortenson/quickedit_image

samuel.mortenson’s picture

Here's how the basic flow works.

Wim Leers’s picture

That looks great! Would love to get that into core :)

samuel.mortenson’s picture

@wim-leers Thanks! I should clean up the documentation and add tests before pulling any patches - but would this be appropriate as its own module (i.e. as it is now), or should it live in Quickedit?

Wim Leers’s picture

Title: Improve the UX of adding images inline » Improve the UX of Quick Editing images

It should live in image.module. It'd just be another InPlaceEditor plugin. See the structure:

  1. default: =\Drupal\quickedit\Plugin\InPlaceEditor\FormEditor
  2. editor.module provides \Drupal\editor\Plugin\InPlaceEditor\Editor
  3. THIS ISSUE: image.module provides \Drupal\image\Plugin\InPlaceEditor\Image

i.e. it's for image fields, whose Field Widget is provided by \Drupal\image\Plugin\Field\FieldWidget\ImageWidget, so it's also up to that module to provide a specialized in-place editor.

Wim Leers’s picture

Issue tags: -Needs design

Per #12, this obviously already has design! Would be so great to have this in 8.2 :)

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

Embedded the GIF in #12 in the IS. The IS will need to be updated further though, to reflect the new direction.

Wim Leers’s picture

Issue tags: +Needs tests

And #12 already exists (at https://github.com/mortenson/quickedit_image), but this would definitely still need tests.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix to the correct media tag.

samuel.mortenson’s picture

Working on a patch without tests to get the issue started, since it seems like there's a lot of interest.

Wim Leers’s picture

Awesome! :)

samuel.mortenson’s picture

Here's a first-shot patch, I can work on tests too but it seems like other people were interested in taking a crack at that. If no one claims that task by tomorrow I can start writing something.

webchick’s picture

Status: Active » Needs review

Yay!!

Status: Needs review » Needs work

The last submitted patch, 22: quickedit-image-2421427-21.patch, failed testing.

Wim Leers’s picture

<3 samuel.mortenson <3

I'll review tomorrow.

Wim Leers’s picture

Issue tags: +JavaScript

This will definitely need review from a JS maintainer like @nod_.

In the mean time, here's my high-level feedback. There is room for making things more robust, and complying with all the nitpicky things that core complies with, but overall, this already looks great!

  1. +++ b/core/modules/image/image.routing.yml
    @@ -71,3 +71,29 @@ image.effect_edit_form:
    +image.upload:
    +  path: '/quickedit/image/upload/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
    ...
    +image.info:
    +  path: '/quickedit/image/info/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
    

    I think it'd make sense to merge these in a single /quickedit/image/{entity_type}/… route, with GET used for reading the info, and POST for changing it.

  2. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,351 @@
    +      // the Quickedit Field Form.
    

    s/Quickedit/Quick Edit/

  3. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,351 @@
    +          var $dropzone = this.renderDropzone('upload', Drupal.t('Drag file here or click to upload'));
    

    Where is this element (and its event handlers) being removed?

  4. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,351 @@
    +            var $input = $('<input type="file">').trigger('click');
    ...
    +      var $toolbar = $('.quickedit-image-field-info');
    

    These global selectors make me nervous. We should do scoped searching, i.e. within the this.$el. In other words, this should use this.$el.find(SELECTOR) instead. That'd remove all risk of potentially changing other elements (outside of a Quick Edit context) that happen to use the same classes.

  5. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,351 @@
    +    ajax: function (type, url, data, callback) {
    

    Why provide a new utility instead of using Drupal.ajax? I'm sure you have a good reason, but let's document that in the docblock for this function then.

  6. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,351 @@
    +      var $dropzone = this.$el.find('.quickedit-image-dropzone');
    

    So, like this :)

  7. +++ b/core/modules/image/src/Controller/QuickeditImageController.php
    @@ -0,0 +1,208 @@
    +   *   The Ajax response.
    

    s/Ajax/JSON/

  8. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -22,6 +22,9 @@
    + *   quickedit = {
    + *     "editor" = "image"
    
    +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -23,6 +23,9 @@
    + *   quickedit = {
    + *     "editor" = "image"
    

    Since this is modifying metadata, you'll need to provide an empty update hook in image.install, to clear caches, to ensure that existing sites start using this new default in-place editor :)

  9. +++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
    @@ -0,0 +1,42 @@
    +    if ($field_definition->getFieldStorageDefinition()->getCardinality() == 1
    +      && $field_definition->getType() == 'image') {
    

    Let's use strict equality here.

  10. +++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
    @@ -0,0 +1,42 @@
    +      return TRUE;
    +    }
    +    return FALSE;
    

    Also, you can just return this condition. So rather than if (CONDITION==TRUE) { return TRUE} return FALSE, you can just do return CONDITION

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
27.43 KB
3.53 KB

#26.1 My only hesitation with this is that you would be GET'ing field settings, and POST'ing the new file upload, so from an API perspective it's a bit confusing. If you'd like, I could try to come up with a generic object that could be passed both ways, containing the file and its metadata (alt/title/etc).

#26.2 Done, I make this mistake all the time.

#26.3 I believe that Drupal.quickedit.EditorView.save() replaces the contents of the entire View with the new field, which would remove the dropzone element. That said, event handlers are not explicitly removed. Should I be doing this before Drupal.quickedit.EditorView.save() is called?

#26.4 Done.

#26.5 Drupal.ajax is, for the most part, intended to be used in correlation with the Form API. Since this editor doesn't use the Form API and the routes return JSON objects that don't include things like Drupal.AjaxCommands, Drupal.ajax isn't the best fit. The reason I architected the editor this way is so that, at some point, the routes could be made more generic and used by non-Drupal/Quickedit specific components. An example of this is CKEditor, which supports drag+drop file uploads. Drupal 8 doesn't allow this yet in its WYSIWYG implementation, but it could easily be added in a follow up issue, given the logic being worked on here. Tying these routes to the Form API and Drupal's AJAX system would make that integration more difficult.

#26.6 Noted!

#26.7 Done.

#26.8 Done, I only added an update to image.install since responsive_image is dependent on image, and wouldn't need its own cache flush.

#26.9/#26.10 Done.

Status: Needs review » Needs work

The last submitted patch, 27: quickedit-image-2421427-27.patch, failed testing.

Wim Leers’s picture

#26.1: having looked at the code closer, you're absolutely right. Never mind me :)

#26.3: I'd have to dive deeper into the code to make a firm recommendation. But I think it'd be at least great to document where these are being removed.

#26.5: Great! Can you move that explanation into the code as a comment, so that future readers/maintainers of the code know this too? :)

#26.8: Yep, perfect!

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
27.93 KB
3.19 KB

Added clearer documentation per #29.

Status: Needs review » Needs work

The last submitted patch, 30: quickedit-image-2421427-30.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
27.93 KB

Ran into case insensitivity issues with Git, re-created #30 with the correct file name.

Status: Needs review » Needs work

The last submitted patch, 32: quickedit-image-2421427-32.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
32.63 KB
6.25 KB

Added library override in the stable theme, which should pass tests.

Edit: Some permission changes got thrown into the patch by accident, but as we still need to write tests for this I'm not too worried. Please ignore this when reviewing the patch. Sorry!

Bojhan’s picture

Looking great, hope we can get this in!

Wim Leers’s picture

Looking great, hope we can get this in!

Does this mean we already have usability sign-off?

Bojhan’s picture

I guess I could have stated it more official :P. Yes!

Wim Leers’s picture

Alright, in excitement, here's another review, this time with lots of nitpicks!

The key thing that we still need, is tests. You could quite easily test the two new routes using classic tests. But we should also test the JS in at least a superficial way (requiring perfect test coverage for that is unreasonable, since Quick Edit itself has zero JS test coverage, because JS tests didn't exist yet back then). You can find a bunch of examples in core by searching for JavascriptTestBase.

  1. --- /dev/null
    +++ b/core/modules/image/css/editors/image.css
    

    This needs to be split in functional styles and theme styles.

    And then the theme styles should be moved into Classy perhaps? Not sure about that, but I'm certain about the splitting.

  2. +++ b/core/modules/image/css/editors/image.css
    @@ -0,0 +1,115 @@
    +  display: flex;
    

    Flexbox! First time we are getting that into core!

  3. +++ b/core/modules/image/css/editors/image.css
    @@ -0,0 +1,115 @@
    +.quickedit-image-dropzone.hover {
    

    Why a hover class? Why not the pseudoclass?

  4. +++ b/core/modules/image/css/editors/image.css
    @@ -0,0 +1,115 @@
    +.quickedit-image-dropzone * {
    

    Woah. That's a scary selector. Wouldn't be using > * be better?

  5. +++ b/core/modules/image/image.install
    @@ -61,3 +61,10 @@ function image_requirements($phase) {
    +function image_update_8001() {
    

    8201, since this is for 8.2.x

  6. +++ b/core/modules/image/images/error.svg
    @@ -0,0 +1,4 @@
    \ No newline at end of file
    

    Let's add a newline to satisfy git

  7. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +  Drupal.quickedit.editors.image = Drupal.quickedit.EditorView.extend(/** @lends Drupal.quickedit.editors.image# */{
    

    The @lends comment is wrong

  8. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +     *   Options for the plain text editor.
    

    c/p remnant

  9. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +      // the Quick Edit Field Form.
    ...
    +          // the document (usually opens them in the same tab).
    

    The "the" fits on the previous line

  10. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +    template_errors: _.template(
    +      '<div class="quickedit-image-errors">' +
    +      '  <%= errors %>' +
    +      '</div>'
    +    ),
    +
    +    /**
    +     * Template to display the dropzone area.
    +     */
    +    template_dropzone: _.template(
    +      '<div class="quickedit-image-dropzone <%- state %>">' +
    +      '  <i class="quickedit-image-icon"></i>' +
    +      '  <span class="quickedit-image-text"><%- text %></span>' +
    +      '</div>'
    +    ),
    +
    +    /**
    +     * Template to display the toolbar.
    +     */
    +    template_toolbar: _.template(
    +      '<form class="quickedit-image-field-info">' +
    +      '<% if (alt_field) { %>' +
    +      '  <label for="alt" class="<% if (alt_field_required) { %>form-required<%} %>">Alt</label>' +
    +      '  <input type="text" placeholder="<%- alt %>" value="<%- alt %>" name="alt" <% if (alt_field_required) { %>required<%} %>/>' +
    +      '<% } %>' +
    +      '<% if (title_field) { %>' +
    +      '  <label for="title" class="<% if (alt_field_required) { %>form-required<%} %>">Title</label>' +
    +      '  <input type="text" placeholder="<%- title %>" value="<%- title %>" name="title" <% if (title_field_required) { %>required<%} %>/>' +
    +      '<% } %>' +
    +      '</form>'
    +    ),
    

    I think these must use Drupal.theme(), so that themes can override these JS templates. With Backbone/Underscore templates, that's not possible.

    Unless you can justify this, of course.

  11. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +            if(e.originalEvent.dataTransfer && e.originalEvent.dataTransfer.files.length){
    

    Violates Drupal core's eslint.

  12. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +          // Before we submit, validate the alt/title text fields.
    +          this.save(options);
    

    Calling this.save() for validation looks strange

  13. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +     * Drupal.ajax is not called here as the Form API is unused by this editor,
    +     * and our JSON requests/responses try to be editor-agnostic. Ideally
    

    s/editor/in-place editor/

  14. +++ b/core/themes/stable/stable.info.yml
    @@ -98,6 +98,10 @@ libraries-override:
    diff --git a/sites/README.txt b/sites/README.txt
    
    diff --git a/sites/README.txt b/sites/README.txt
    old mode 100644
    new mode 100755
    diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
    
    diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
    old mode 100644
    new mode 100755
    diff --git a/sites/development.services.yml b/sites/development.services.yml
    
    diff --git a/sites/development.services.yml b/sites/development.services.yml
    old mode 100644
    new mode 100755
    diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php
    
    diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php
    old mode 100644
    new mode 100755
    diff --git a/sites/example.sites.php b/sites/example.sites.php
    
    diff --git a/sites/example.sites.php b/sites/example.sites.php
    old mode 100644
    new mode 100755
    

    These mode changes need to be removed :)

samuel.mortenson’s picture

#38.1: Done. Both stylesheets are duplicated in stable, which should trickle down to most themes.

#38.2: I don't dare attempt vertical+horizontal centering without it. ;)

#38.3: The "hover" class is unique to ":hover" in that it only is added if a user is hovering a file over the dropzone area. This allows us to indicate that an area is "droppable" in a unique way to just hovering over the dropzone with a mouse.

#38.4: It is scary, but it is necessary to avoid a host of issues with drag+drop. Without a fix like this, if a user drops a file onto the icon or text inside the dropzone, the *drop events will be triggered on those elements and not the dropzone, which is what we have events bound to. This usually results in the browser redirecting to the file, which is not good. See http://stackoverflow.com/questions/10867506/dragleave-of-parent-element-... for more info.

#38.5/#38.6: Done!

#38.7: I'm not sure what's wrong with it exactly, but I'm copying what Quickedit is doing for all of its Backbone Views and Models. See http://cgit.drupalcode.org/drupal/tree/core/modules/quickedit/js/views/A... for what I based this off of.

#38.8: Done.

#38.9: If I put "the" on the previous line, it will be 81 characters long.

#38.10: Why not both? I went ahead and moved all the templates to the Drupal.theme namespace, but kept the Underscore template style. Users can choose to override these with raw functions if they wish.

#38.11: Done, and I fixed a few other ESLint errors.

#38.12: The comment was outdated, the save method is not overridden.

#38.13: Done!

#38.14: Sorry about that, I did mention it in my comment but knew it would come up.

Sadly I'm going offline for the weekend so I won't be able to write tests just yet, but I'm dedicated to doing this next week. If anyone wants to take the reigns on this, I won't mind.

Wim Leers’s picture

Awesome :) I'm hoping @nod_ will have time to also review this. Ideally, I should be able to provide a more thorough review of the JS, but I haven't touched Quick Edit's JS in well over a year (if not two). So I'm personally actually already okay with this code, it's sufficiently maintainable already. The only thing that is an absolute must-have still is test coverage.

samuel.mortenson’s picture

dawehner’s picture

+++ b/core/modules/image/src/Tests/QuickEditImageTest.php
@@ -0,0 +1,138 @@
+class QuickEditImageTest extends ImageFieldTestBase {

Given that this is a JS based feature we should also have a JS based testing, see #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary

Wim Leers’s picture

Agreed. The most relevant example to look at is probably \Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest.

samuel.mortenson’s picture

I agree, but with Quick Edit already having no Javascript test coverage I'm wondering if that should be a follow up issue.

dawehner’s picture

Well, we are talking about a feature request, aren't we :)

Wim Leers’s picture

I hear you. But the rule is: "nothing new that uses JS gets added without tests". Otherwise the "lack of tests" problem just becomes bigger.

You don't need to test every possible edge case. Just test the common case. It'd be unreasonable to require you to have test coverage for every Quick Edit edge case, that'd indeed be out of scope.

samuel.mortenson’s picture

Got it, I'll start re-writing the test.

Edit: I actually like the test I already have, it makes sure the controller works without taking the editor into account. I'll write a new Javascript test for the most basic drag+drop upload behavior.

Wim Leers’s picture

Yes! Keep the current test — it tests the endpoints, which is great. Just add one to test the JS code too.

samuel.mortenson’s picture

Here's a basic Javascript test which covers the standard upload flow that this feature provides. To accomplish this I had to move some code around and create new traits, as well as researching a way to mock the drag and drop behavior since we're using a headless browser and not something like Selenium. Hopefully the Quick Edit bits inspire a base class and larger test coverage for Quick Edit in the future!

Status: Needs review » Needs work

The last submitted patch, 49: quickedit-image-2421427-49.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
60.05 KB
644 bytes

Looks like the pesky GastonJS bug I found while working on this popped up again. Here's a small change that might fix it (locally the test passes for me with or without this).

Status: Needs review » Needs work

The last submitted patch, 51: quickedit-image-2421427-51.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
59.87 KB
936 bytes

So after some troubleshooting and screenshot-ing, I found that the inconsistent part of the tests was Quick Edit itself. Debugging the HTML output of the session right before failure showed either "Your changes to Test Node could not be saved" or "Could not load the form for ..." messages coming from Quick Edit after the "Save" button was pressed. Everything up to that point worked consistently without errors.

Now the test as it stands now will cover everything up to the point of saving, but will not actually save the Node. This shows that the Javascript and Controller works, but of course doesn't prove that the new Image is really set on the Node. Unless someone knows a path forward, I would suggest troubleshooting the Quick Edit weirdness when we actually add Javascript test coverage for that module.

Status: Needs review » Needs work

The last submitted patch, 53: quickedit-image-2421427-53.patch, failed testing.

Wim Leers’s picture

Issue tags: -Needs tests

To accomplish this I had to move some code around and create new traits, as well as researching a way to mock the drag and drop behavior since we're using a headless browser and not something like Selenium.

Sounds tricky but great!

Hopefully the Quick Edit bits inspire a base class and larger test coverage for Quick Edit in the future!

+1 :) I'm certain this will help!


#53: +1 for that scope of testing. This tests this in-place editor, nothing more. That's fine.

However, what I'd still like to see this testing, is entering a value for alt text, and then verifying that it's there. Clicking the image again, to modify its alt text, then verify that the modified alt text is present. Then I think this is testing everything of this in-place editor.

  1. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,144 @@
    +    if ($this->profile != 'standard') {
    +      $this->drupalCreateContentType(['type' => 'article', 'name' => 'Article']);
    +    }
    

    This will always be true. Let's move this $this->drupalCreateContentType() call into a setUp() method.

  2. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,144 @@
    +    $admin = $this->drupalCreateUser(['access contextual links', 'access in-place editing', 'access content', 'access administration pages', 'administer site configuration', 'administer content types', 'administer node fields', 'administer nodes', 'create article content', 'edit any article content', 'delete any article content', 'administer node display']);
    +    $this->drupalLogin($admin);
    

    These can also be moved into a setUp() method.

  3. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,144 @@
    +    // Initiate Quick Editing.
    +    $this->triggerClick($entity_selector . ' [data-contextual-id] > button');
    +    $this->click($entity_selector . ' [data-contextual-id] .quickedit > a');
    +    $this->click($field_selector);
    

    Elegant :)

  4. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,144 @@
    +    // Our headless browser can't drag+drop files, but we can mock the event.
    +    // Append a hidden upload element to the DOM.
    +    $script = 'jQuery("<input id=\"quickedit-image-test-input\" type=\"file\" />").appendTo("body")';
    +    $this->getSession()->executeScript($script);
    +
    +    // Find the element, and set its value to our new image.
    +    $input = $this->assertSession()->elementExists('css', '#quickedit-image-test-input');
    +    $filepath = $this->container->get('file_system')->realpath($valid_images[1]->uri);
    +    $input->attachFile($filepath);
    +
    +    // Trigger the upload logic with a mock "drop" event.
    +    $script = 'var e = jQuery.Event("drop");'
    +      . 'e.originalEvent = {dataTransfer: {files: jQuery("#quickedit-image-test-input").get(0).files}};'
    +      . 'e.preventDefault = e.stopPropagation = function() {};'
    +      . 'jQuery(".quickedit-image-dropzone").trigger(e);';
    +    $this->getSession()->executeScript($script);
    

    Woah, clever!

  5. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,144 @@
    +    $this->assertSession()->elementNotExists('css', 'img[src*="' . $valid_images[0]->filename . '"]');
    +    $this->assertSession()->elementExists('css', 'img[src*="' . $valid_images[1]->filename . '"]');
    

    I think these selectors include the Quick Edit entity + field selectors, to ensure these have appeared in the expected place.

Wim Leers’s picture

I also just manually tested this and MAN THIS IS AWESOME!!!!!!!!!!

Wim Leers’s picture

Issue summary: View changes
FileSize
146.25 KB

Really the only remark I have on the UX is that the label says Alt instead of Alternative text. The image widget says Alternative text. I think this should too. For consistency, but also understandability: Alt sounds too tech-y, too programmer-y for a content author.

Wim Leers’s picture

#55 is a review of just the JS test coverage.

Here's a review of the entire patch. Basically only nits. This patch looks very close to ready to me :) I think this is looking very likely to still land in 8.2!

  1. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +   * Theme function for validation errors of the Image module's in-place
    +   * editor.
    ...
    +   * Theme function for the dropzone element of the Image module's in-place
    +   * editor.
    

    Must fit in 80 cols.

    I'd omit the word "module's". Just "the Image in-place editor" is fine, and actually is even more accurate.

  2. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +          var $dropzone = this.renderDropzone('upload', Drupal.t('Drag file here or click to upload'));
    

    Shouldn't Drag be Drop here?

  3. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +            if(e.originalEvent.dataTransfer && e.originalEvent.dataTransfer.files.length) {
    

    A single small eslint error here :)

  4. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +            var $input = $('<input type="file">').trigger('click');
    

    This is creating an <input type=file> without putting it in the DOM, right? Perhaps a brief comment here would be valuable.

  5. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +      this.renderDropzone('upload loading', Drupal.t('Uploading <i>@file</i>...', {'@file': file.name}));
    

    Nit: these are 3 points, should be an ellipsis: , not ....

  6. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +        // Indicate that the field has changed - this enables the
    +        // "Save" button.
    ...
    +        // Replace our innerHTML with the new image. If we replaced
    +        // our entire element with data.html, we would have to
    +        // implement complicated logic like what's in
    +        // Drupal.quickedit.AppView.renderUpdatedField.
    

    This can be reformatted a bit to put more on the same line.

  7. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,397 @@
    +     * in-place editor, and our JSON requests/responses try to be editor
    +     * agnostic. Ideally similar logic and routes could be used by modules
    

    Super nit: s/editor agnostic/editor-agnostic/

  8. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,222 @@
    +   * Returns a JSON object representing the new file upload, or validation
    +   * errors.
    

    Must fit in 80 cols.

    Suggestion: s/a JSON object/JSON/

  9. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,222 @@
    +      return new JsonResponse(['errors' => $this->renderer->render($messages), 'main_error' => $this->t('The requested image failed validation.')]);
    

    This message looks a bit weird to me. It's not really a requested image, is it? It's an uploaded image. It's really just an image. So what about The image failed validation.?

Bojhan’s picture

@Wim I also never understood why its "Alternative". Its the only text :D, but anyway guess we should just follow the spec.

Wim Leers’s picture

Heh :) We can open a separate issue to rename Alternative text to Text in a separate issue for all places in drupal core where it is used if you want :)

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
61.46 KB
12.64 KB

All review comments addressed, and tests fixed (with the Quick Edit save added back in!) locally.

Wim Leers’s picture

  1. +++ b/core/modules/image/css/editors/image.theme.css
    @@ -0,0 +1,88 @@
    +  -webkit-user-select: none;
    

    Don't we also need the moz and ms ones? And perhaps also the unprefixed (yet unofficial) one?

  2. +++ b/core/modules/image/image.routing.yml
    @@ -71,3 +71,29 @@ image.effect_edit_form:
    +    _permission: 'access in-place editing'
    +    _access_quickedit_entity_field: 'TRUE'
    ...
    +    _permission: 'access in-place editing'
    +    _access_quickedit_entity_field: 'TRUE'
    

    Great! But there's no test coverage for this in QuickEditImageControllerTest. A simple check with an anonymous user to verify that they get 403 responses is sufficient.

  3. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,221 @@
    +   * Stores the Quick Edit tempstore.
    

    Nit: s/Stores the/The/

  4. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,221 @@
    +   *   The Ajax response.
    

    s/Ajax/JSON/

  5. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,221 @@
    +    return new JsonResponse($info);
    

    This should be \Drupal\Core\Cache\CacheableJsonResponse, and needs the appropriate cache tags.

  6. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,221 @@
    +   * Returns a JSON object representing the current state of the field.
    

    Wrong comment.

  7. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,221 @@
    +   * @param string $langcode
    +   *   The language code of the field that is being rendered.
    +   * @return \Drupal\image\Plugin\Field\FieldType\ImageItem
    

    Nit: We need an empty line between the last @param and the @return.

  8. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,177 @@
    +    // Login as an admin user who can use Quick Edit and edit Articles.
    

    Nit: s/Login/Log in/

  9. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,177 @@
    +    $this->adminUser = $this->drupalCreateUser(['access contextual links', 'access in-place editing', 'access content', 'access administration pages', 'administer site configuration', 'administer content types', 'administer node fields', 'administer nodes', 'create article content', 'edit any article content', 'delete any article content', 'administer node display']);
    

    Let's put these all below each other.

    Also, let's rename this from $this->adminUser to $this->contentAuthorUser or something like that, and remove all the administrative permissions?

Status: Needs review » Needs work

The last submitted patch, 61: quickedit-image-2421427-61.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
62.84 KB
8.1 KB

All notes addressed, again. :-)

#62.9 Was really helpful since it uncovered that QuickEditImageControllerTest didn't need to extend ImageFieldTestBase, as it wasn't testing any Drupal forms. Now the setUp method of both tests are in sync.

For the test failure - I can't replicate it locally, but it could be that the wait time isn't long enough for the upload. I bumped it up, but if this fails for the same reason I'll probably need to debug the TestBot.

Status: Needs review » Needs work

The last submitted patch, 64: quickedit-image-2421427-64.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
62.93 KB
912 bytes

I have a new theory - in Drupal.quickedit.editors.image.uploadImage, an AJAX POST is made, then when it finishes successfully the dropzone element is removed. So really, we need to do two waits - one for AJAX, one for the dropzone element to be removed.

Status: Needs review » Needs work

The last submitted patch, 66: quickedit-image-2421427-66.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
62.71 KB
752 bytes

Well that theory was wrong - let's remove what's failing now and go back to a normal AJAX wait. From there we can debug the next failure.

Status: Needs review » Needs work

The last submitted patch, 68: quickedit-image-2421427-68.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
63.67 KB
854 bytes

This is a patch triggered by my daily dealing with APCU

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Reproduced locally (with APC enabled or disabled, doesn't matter):

wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ sudo -u _www php ./vendor/bin/phpunit -c core --group image --testsuite functional-javascript
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

F

Time: 35.4 seconds, Memory: 7.25Mb

There was 1 failure:

1) Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest::testUpload
Javascript condition met:
jQuery('[data-quickedit-field-id="node/1/twoxvvug/en/full"] .quickedit-image-dropzone').length == 0
Failed asserting that false is true.

/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php:102
/Users/wim.leers/Work/drupal-tres/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php:153

FAILURES!
Tests: 1, Assertions: 11, Failures: 1.
wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ apcu off
Disabling APCu…
wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ sudo -u _www php ./vendor/bin/phpunit -c core --group image --testsuite functional-javascript
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

F

Time: 33.03 seconds, Memory: 7.25Mb

There was 1 failure:

1) Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest::testUpload
Javascript condition met:
jQuery('[data-quickedit-field-id="node/1/ygzg3zlu/en/full"] .quickedit-image-dropzone').length == 0
Failed asserting that false is true.

/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php:102
/Users/wim.leers/Work/drupal-tres/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php:153

FAILURES!
Tests: 1, Assertions: 11, Failures: 1.

Trying to figure out a fix.

Status: Needs review » Needs work

The last submitted patch, 70: 2421427-70.patch, failed testing.

Wim Leers’s picture

I'm pretty certain that I've found the reason it is not working for testbot nor me, but is for @samuel.mortenson. Phantomjs.

By including #2702661: Make it simple to take screenshots whilst using JavascriptTestBase's patch and taking a screenshot, you'll see that the input[type=file] continues to say "Choose file". (This patch/test uses that just to simulate a dragged-and-dropped file.)

So I started searching, and found http://stackoverflow.com/questions/29916779/file-upload-with-behat-mink-..., which points to https://github.com/ariya/phantomjs/issues/12506. It was fixed in phantomjs 2.1 (https://github.com/ariya/phantomjs/milestone/14?closed=1), released on Feb 3, 2016.

On my machine, I have this version:

wim.leers at wimleers-acquia in ~/Work/drupal-unus on 8.1.x*
$ brew info phantomjs
phantomjs: stable 2.1.1 (bottled), HEAD
Headless WebKit scriptable with a JavaScript API
http://phantomjs.org/
/usr/local/Cellar/phantomjs/2.0.0 (58 files, 50M) *
  Poured from bottle on 2015-02-02 at 14:43:39
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/phantomjs.rb

… which says "stable 2.1.1", but then later it says "2.0.0", but really it's been compiled in February 2015, and the fix only landed in December 2015. (Oh, I guess "stable 2.1.1" means that that is the current stable version.)

After updating (to 2.1.1), this is what happens:

wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ sudo -u _www php ./vendor/bin/phpunit -c core --group image --testsuite functional-javascript
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

.

Time: 11.68 seconds, Memory: 7.25Mb

OK (1 test, 11 assertions)

So, testbot must be running Phantomjs 2.0.x, instead of Phantomjs 2.1.x.

EDIT: samuel.mortenson confirmed: he's running PhantomJS 2.1.1

Wim Leers’s picture

Talked to testbot maintainer @Mixologic. He confirms testbot is currently on PhantomJS 2.0. So that's definitely the explanation then. He says updating PhantomJS may be possible in the next few days.

See #2773637: Update PhantomJS to 2.1.1.

Wim Leers’s picture

The only possible work-around is to create a File object ourselves. See https://www.w3.org/TR/file-upload/#dfn-filelist. Concrete example at the bottom of http://thinkmoult.com/using-sahi-mink-behat-test-html5-drag-drop-file-up.... This works in Chrome for example:

new File([new Blob()], "name.png");

But in PhantomJS 2.0, and 2.1, and HEAD, this is what you get:

TypeError: FileConstructor is not a constructor (evaluating 'new File([new Blob()], "name.png")')

PhantomJS issue: https://github.com/ariya/phantomjs/issues/14247.

So, alas, there's no work-around possible.

samuel.mortenson’s picture

samuel.mortenson’s picture

I know the TestBot hasn't been updated yet, but I wanted to re-post my patch from #64 since the last three patches were just debugging the TestBot.

Status: Needs review » Needs work

The last submitted patch, 77: quickedit-image-2421427-77.patch, failed testing.

The last submitted patch, 77: quickedit-image-2421427-77.patch, failed testing.

The last submitted patch, 77: quickedit-image-2421427-77.patch, failed testing.

droplet’s picture

Fixing tests

samuel.mortenson’s picture

@droplet Awesome work! Your debugging on the triggerClick() problem has been great, and seeing the hack removed here with tests passing is very validating.

Now I'm just waiting for more review. :-)

Mixologic’s picture

Yep. I see you see that the testbot is now updated. Carry on.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community

WOOT! GREEN!

This feature is perfectly isolated: it's a new Quick Edit in-place editor, plus supporting files. It can therefore be easily reverted during beta or RC if problems are uncovered.

IMO:

  1. the risk is contained
  2. the benefit is huge (just see the animated GIF in the issue summary)
  3. it was ready in time, but was blocked on infrastructure
  4. therefore, still worthy of committing it to 8.2.x
droplet’s picture

Let's vote for REALTIME debugging.
#2775653: [PP-2] JavascriptTests with webDriver

and make Drupal Tests compatible with Psysh, then we able to do live code & testing Tests.

(Just a note, I didn't review any of this patch. I'm fixing the tests only.)

phenaproxima’s picture

For what my opinion is worth, which is probably nothing...+1 to what Wim said in #84. This is a valuable, low-risk feature and would be a big win for 8.2.

slashrsm’s picture

+1 for RTBC. Great improvement and isolated enough to be a save thing to add.

Great work @Samuel.mortenson!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/src/Tests/ImageFieldCreationTrait.php
@@ -0,0 +1,68 @@
+trait ImageFieldCreationTrait {

+++ b/core/modules/simpletest/src/TestFilesTrait.php
@@ -0,0 +1,117 @@
+trait TestFilesTrait {

This can occur in a blocking patch. At 60k this patch is big enough and these changes distract from the context.

IN #26 @Wim Leers asks for a review from a JS maintianer like @_nod that still does not appear to have occurred. @droplet notes in #85 that they have not reviewed any of the patch,

alexpott’s picture

Issue tags: +CSS

It would also be great to get a CSS maintainer's eyes on the issue - maybe @Cottser?

webchick’s picture

Wim is on honeymoon for the next 3 weeks, so if someone else is able to re-roll per @alexpott's comments, that'd be awesome.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

samuel.mortenson’s picture

Here's a shot at the refactor suggested in #88. I filed #2782309: Refactor File and Image related image field creation logic into a new trait for the new traits, and re-rolled the patch from #81 to not include any of the trait changes.

Keeping the issue in Needs Work as it will fail tests until the new issue is committed.

samuel.mortenson’s picture

Issue tags: +beta target
xjm’s picture

Issue tags: -beta target

@samuel.mortenson, only committers should add the beta target tag. (We have some disagreement about whether this issue can be one.) Thanks!

xjm’s picture

Status: Needs work » Needs review

Triggering testbot.

xjm’s picture

@samuel.mortenson, one strategy we can use is to combine "-do-not-test" patches containing only the changes for this issue with full patches that include the blockers, to prove tests pass. Otherwise we should postpone this issue on the other.

Status: Needs review » Needs work

The last submitted patch, 92: quickedit-image-2421427-92.patch, failed testing.

samuel.mortenson’s picture

@xjm Here's the full patch from #81 and a do-not-test patch without the trait refactoring.

Sorry for adding the "beta target" tag prematurely, I inferred from @wim-leers and @webchick's feedback that there was consensus among core committers. Won't happen again!

The last submitted patch, 98: quickedit-image-2421427-98-no-traits-do-not-test.patch, failed testing.

nod_’s picture

Reviewing

nod_’s picture

Status: Needs review » Needs work

Lost my comment mid way so a quick version and i'll finish later.

  1. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +      this.model.set('originalValue', $.trim(this.$el.html()));
    

    there is a native function for this. If it's trimmed, it's not really the original value no?

  2. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +          var stopEvent = function (e) {
    +            e.preventDefault();
    +            e.stopPropagation();
    +          };
    

    I'd rather keep things explicit and have preventDefault and stopPropagation everywhere.

  3. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +          $dropzone.on('dragleave', function (e) {
    +            stopEvent(e);
    

    It's better do to the preventDefault and stopPropagation after any handling code. In case the code breaks.

  4. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +            $input.change(function () {
    

    Always use .on(), not the jquery event aliases

  5. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +      var data = new FormData();
    

    This is not available on IE9/10, I'm fine with it but that might be a problem.

  6. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +        $el.html(content);
    

    Might need a Drupal.attachBehaviors no?

  7. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +    ajax: function (type, url, data, callback) {
    

    Why not make it like the jquery ajax function and let it accept an object. that would prevent the need to pass in null later on.

    $.extends() can be use to merge the default and provided values.

  8. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,402 @@
    +        $dropzone = $(Drupal.theme.quickeditImageDropzone({
    

    We're supposed to use it like Drupal.theme('quickeditImageDropzone', {}) but it's not a big problem

nod_’s picture

Status: Needs work » Needs review
FileSize
16.14 KB
63.35 KB

Yep, does look much better.

Addressed all my issues, Need an descision from core comitters on the IE10 not supporting FormData.

underscore templates are not necessary, it doesn't take much more code to do it in JS and we don't introduce a depencency on underscore templates. If js templating (as in using dedicated templating language) happens in D8 it need to be a dedicated decision, not sneaked in in an unrelated patch.

samuel.mortenson’s picture

Any word from core maintainers about whether or not this is going to be targeted for the 8.2.x beta cycle?

Gábor Hojtsy’s picture

@samuel.mortenson: the last beta was released for Drupal 8.2, so no chance there. RC1 is coming out next week, see https://groups.drupal.org/node/513529. Improvements are already committed to 8.3, eg. the views listing page is already significantly different in 8.3 from 8.2, so this has a good chance to land early in 8.3 and get released next April.

samuel.mortenson’s picture

Version: 8.2.x-dev » 8.3.x-dev

Thanks! Updating the issue to reflect that.

Wim Leers’s picture

Sad to see this didn't make it in 8.2. Of course, we need to stop somewhere. Glad to see this is extremely likely to be in 8.3!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Need an descision from core comitters on the IE10 not supporting FormData.

Actually, IE10 does support FormData. It's just buggy, and the bugs are only being fixed in Edge (not even in IE11). IE10 currently has 0.57% global market share — even less than IE9! And it's unsupported since January 12, 2016.

I'll be bold.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 102: core-quickeditimage-2421427-102.patch, failed testing.

samuel.mortenson’s picture

Fixed minor patch conflict.

samuel.mortenson’s picture

The last submitted patch, 109: core-quickeditimage-2421427-109.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reroll!

xjm’s picture

Assigned: Unassigned » Cottser

Nice, glad to see this RTBC again.

Looks like @alexpott asked for @Cottser's feedback on the CSS, so assigning.

samuel.mortenson’s picture

There is some movement in other issues around doing similar File/Image test refactors as this issue - so we should try to incorporate #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile when eventually committing this issue.

Wim Leers’s picture

If this issue would just land, then that issue can just update the tests added by this patch. :)

Cottser’s picture

Thanks for the bumps, hoping to get to this soon.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 110: quickedit-image-2421427-110.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs reroll
Wim Leers’s picture

Priority: Normal » Major

As of #2235977-68: JS Client-side file validation is broken (because ajaxPageState is broken?), that major bug's patch is now also depending on the JS test trait that this issue adds.

Also, this issue is a very big usability win.

For both of those reasons, bumping this to major.

samuel.mortenson’s picture

Re-rolled the patch assuming that #2782309: Refactor File and Image related image field creation logic into a new trait is going to get committed. This patch is smaller than #110 as #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile was already committed.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll
Wim Leers’s picture

Title: Improve the UX of Quick Editing images » [PP-1] Improve the UX of Quick Editing images
Status: Reviewed & tested by the community » Postponed
naveenvalecha’s picture

Title: [PP-1] Improve the UX of Quick Editing images » Improve the UX of Quick Editing images
Status: Postponed » Needs review
naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #121

samuel.mortenson’s picture

As to not confuse reviewers, I've re-named the no-traits patch and re-uploaded it.

Wim Leers’s picture

Perfect :)

dawehner’s picture

One thing I'm wondering is: is it okay to add a route which requires another module to be installed.

  1. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,225 @@
    +      if (in_array($view_mode_id, $entity_view_mode_ids)) {
    

    Nitpick: its always better to use string mode when dealing with strings.

  2. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -0,0 +1,225 @@
    +    $field_list = $entity->getTranslation($langcode)->$field_name;
    ...
    +      $field = $entity->getTranslation($langcode)->$field_name->get(0);
    

    Just a personal style: IMHO its just way nicer to use ->get($field_name). Note: get(0) is the same as first()

Wim Leers’s picture

#127: RE: route requiring another module. That's a very good point actually. Perhaps we should conditionally add those routes, only if the quickedit module is installed?

#127.1 s/string/strict/ :D

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
7.28 KB
7.23 KB

Sorry for the delayed review, folks. I'm not sure if the routing point brought up in #127 needs addressing before commit or not.

+++ b/core/modules/image/css/editors/image.theme.css
@@ -0,0 +1,90 @@
+  margin-right: 10px;
...
+  margin-right: 0;

I think these need /* LTR */ comments and relevant RTL code added (see screenshots below). https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

LTR:

RTL:

tim.plunkett’s picture

+++ b/core/modules/image/image.routing.yml
@@ -71,3 +71,29 @@ image.effect_edit_form:
+    _access_quickedit_entity_field: 'TRUE'
...
+    _access_quickedit_entity_field: 'TRUE'

This isn't necessarily a security issue, since anyone hitting this route should get a ServiceNotFoundException. But yes, these should be added dynamically.

Wim Leers’s picture

NW for #127.1 (it's already safe as #130 explains, but it's clearer) + #129.

samuel.mortenson’s picture

I based my route definition on the editor module's in-place editor route, which is not added dynamically:

editor.field_untransformed_text:
  path: '/editor/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
  defaults:
    _controller: '\Drupal\editor\EditorController::getUntransformedText'
  options:
    parameters:
      entity:
        type: entity:{entity_type}
  requirements:
    _permission: 'access in-place editing'
    _access_quickedit_entity_field: 'TRUE'

I'll re-roll today and address #129 and #127, but if we need to address the route issue we should do it for editor as well.

Wim Leers’s picture

but if we need to address the route issue we should do it for editor as well.

Excellent point. Let's defer that to a follow-up then.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
49.98 KB
1.98 KB

This patch addresses the notes in #129 and #127.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Thanks! Only minor things left I think. :)

CSS

One more minor CSS point:

+++ b/core/modules/image/css/editors/image.theme.css
@@ -0,0 +1,99 @@
+  -webkit-user-select: none;
+  -moz-user-select: none;
+  -ms-user-select: none;

We should also include the non-prefixed version. Chrome 54 and higher even support it :)

http://caniuse.com/#search=user-select

PHP

  1. +++ b/core/modules/image/src/Tests/QuickEditImageControllerTest.php
    @@ -0,0 +1,186 @@
    +   * @param integer $nid
    

    Minor: PHPCS wants this to be int, not integer.

    FILE: ...ev/core/modules/image/src/Tests/QuickEditImageControllerTest.php
    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
     157 | ERROR | [x] Expected "int" but found "integer" for parameter
         |       |     type
  2. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    @@ -0,0 +1,171 @@
    +    $this->assertSession()->elementExists('css', $entity_selector . ' ' . $field_selector . ' ' . $new_image_selector);
    +  }
    +}
    

    Minor: Missing blank line before the class' closing brace per PHPCS.

    FILE: ...ules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
     171 | ERROR | [x] The closing brace for the class must have an empty
         |       |     line before it

JavaScript

There are also some eslint errors. The 'use strict' ones seem wrong, maybe something is wrong on my end or we need to update our estlintrc again? The rest seem legitimate though.

/Users/Scott/Sites/d8commit.dev/core/modules/image/js/editors/image.js
  6:1  error  Use the global form of 'use strict'  strict
  8:3  error  Use the global form of 'use strict'  strict

✖ 2 problems (2 errors, 0 warnings)


/Users/Scott/Sites/d8commit.dev/core/modules/image/js/theme.js
   6:1    error  Use the global form of 'use strict'          strict
   8:3    error  Use the global form of 'use strict'          strict
  21:47   error  Missing space before function parentheses    space-before-function-paren
  42:63   error  Infix operators must be spaced               space-infix-ops
  78:118  error  There should be no spaces inside this paren  space-in-parens
  81:14   error  Multiple spaces found before ''</form>''     no-multi-spaces

✖ 6 problems (6 errors, 0 warnings)
samuel.mortenson’s picture

Addressed the notes in #136, except the "use strict" warning, which would also apply to many other Drupal JS files. Guessing that's just a quirk.

I also updated the stable theme's copy of image.theme.css to reflect the changes I made in #134, I missed that last time.

samuel.mortenson’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

The strict thing is indeed a known problem, being fixed at #2821113: eslint 'use strict' rule wrong.

droplet’s picture

Is it really working?

Could anyone teach me how to use it? I remember that's no config when I tested and patched in #81. Now, I can't drag-n-drop.

Tested with simplytest.me also.

samuel.mortenson’s picture

@droplet Yes, the patch still works. To test it, you can install the standard profile, create a new Article with only* the Title and Image fields filled in, then Quick Edit that Article from the front-end. Once Quick Edit is initiated, clicking on the Article's image should prompt you to either click to upload or drag and drop.

* The Bartik theme floats Article images the left, which puts them below the body field, which has 100% width. As a result you cannot Quick Edit Article images in Bartik if the body field is filled in.

droplet’s picture

Ahh. I found the problem.

It doesn't work if I ALSO upload another an image via IMAGE ICON in CKEditor.

EDITED: even only TEXT in body field, it doesn't work

samuel.mortenson’s picture

This patch does not add any functionality to CKEditor, it only adds a new inline editor to single-value Image fields. You should not be using the body field for testing.

Wim Leers’s picture

Title: Improve the UX of Quick Editing images » Improve the UX of Quick Editing single-valued image fields

#142: for that, there is #2560457: Support drag-and-drop image uploads in CKEditor. This issue applies only to Quick Edit. You're referring to CKEditor.

Updated the issue title to make that as explicit as possible.

droplet’s picture

Umm.... any issues for overlay conflicts between 2 fields?

There's a hidden bug in CORE (or this patch)

1. applied patch & update.php & clear caches.
2. GIT reset --hard, clear caches.
3.
ERROR 500
drupal8x/quickedit/attachments?_wrapper_format=drupal_ajax

Looks like doesn't clear the router caching correctly

samuel.mortenson’s picture

Umm.... any issues for overlay conflicts between 2 fields?

Quick Edit has problems when fields are :

  1. Overlapped other fields, which may cause contextual links to stack.
  2. Positioned with a z-index below another element (and as a result are not click-able)
  3. Positioned absolutely

Themes can do lots of things to make Quick Edit inaccessible, and while we could patch Bartik to not use floats for image fields, that is out of scope for this issue and may cause problems for users with Bartik sub-themes.

I haven't seen the bug you pointed out, can this be replicated consistently by installing Drupal, applying the patch, then running update.php?

Cottser’s picture

I don't think I'll have time to re-review this myself in the near future, but another committer can pick it up. I was happy with it overall last I looked :) thanks all!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/image/image.install
--- a/core/modules/image/image.libraries.yml
+++ b/core/modules/image/image.libraries.yml

+++ b/core/modules/image/image.libraries.yml
@@ -3,3 +3,19 @@ admin:
+quickedit.inPlaceEditor.image:
...
+    - quickedit/quickedit

Are we OK with image module providing a library that depends on quickedit? The alternative would be to add this library in hook_library_info_build() (or what it's called) conditionally if quickedit module is installed. Not sure ...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#148: that quickedit.inPlaceEditor.image asset library will only be attached by Quick Edit the module. Just like it's okay to define a service that will only be conditionally picked up by some module, this is a library that will only be conditionally picked up by some module.

So even if we want to move this to hook_library_info_build(), that can absolutely be done in a follow-up.


This patch has been RTBC for >2 months, since #112. Everything since than has been nitpicks. We can keep nitpicking it to death, or we can land it and then just keep improving the Quick Edit module. At this rate, nobody will ever again want to contribute new features to Quick Edit. :(

ipwa’s picture

We can keep nitpicking it to death, or we can land it and then just keep improving the Quick Edit module. At this rate, nobody will ever again want to contribute new features to Quick Edit. :(

I completely agree, I really wanted to see this on Drupal 8.2

tstoeckler’s picture

+++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
@@ -0,0 +1,39 @@
+namespace Drupal\image\Plugin\InPlaceEditor;
...
+ * @InPlaceEditor(
+ *   id = "image"
+ * )
+ */
+class Image extends InPlaceEditorBase {
...
+        'image/quickedit.inPlaceEditor.image',

Ahh I missed, that the library is conceptually attached to the plugin.

In that case, there's no reason for any additional logic, in my opinion.

Thanks for setting me straight!

Wim Leers’s picture

np, and thanks for looking out :) I totally do appreciate the constructive criticism!

  • webchick committed 24eb070 on 8.3.x
    Issue #2421427 by samuel.mortenson, droplet, dawehner, nod_, Cottser,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
428.53 KB

Reviewed this on UX meeting today, since Cottser handed it over for another committer to look at.

While testing, we ran into the same issue as @droplet, where if you upload an image via CKEditor, as well as an image via a regular image field, the Quick Edit target will fail to allow you to target the Image field (only Body). However, this seems to be a pre-existing condition, according to comments by Wim and others.

An image field targeting body instead.

We walked through the patch together and looks good to us. Nice test coverage (including JS tests! :)). Code all seems to be conforming to coding standards (or at least https://github.com/alexpott/d8githooks wasn't triggering anything), and error handling seems to be working well, so we should be good to go here.

Committed and pushed to 8.3.x. Thanks!

xjm’s picture

Issue tags: +8.3.0 release notes

Yay!

xjm’s picture

dmsmidt’s picture

This is so awesome, just tested it, thank you for this!

Happy that this has landed, thus I won't reopen this, but create some follow up issues:

#2827997: Quick Editing single images is impossible when they are floated
#2827996: Drag and drop UX for Quick Editing small images is difficult

RajabNatshah’s picture

Nice, at last this issue is committed!!!

Cottser’s picture

I definitely played a part in delaying this (#149). Sorry to be a bottleneck, but happy to see this in :)

dmsmidt’s picture

Wim Leers’s picture

#161: Thanks! Updated/clarified both of those issues. Thanks so much for digging deep and finding edge cases :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture