Improve Field Widget UX for single-cardinality File/Image fields

Nice improvement! Note that I wasn't able to test this yet so just general feedback.

How this behaves on mobile? I suspect it should work nice but we should check.

Can we add a bit of test coverage (for both notice and field widget UX change)?

With this kind of issues it is nice to have screenshots in the IS. Adding that.

I have the feeling that this issue has become obsolete with the latest improvements. @samuel.mortenson could you please confirm?

At least I'm not able to reproduce the "Before" screenshot indicated above with the latest -dev.

Thanks!

True, thanks for that, and sorry for missing it.

I can confirm that the patch still applies, and that it passes manual testing :)

I also checked that apparently nothing looks broken on a mobile viewport:

Does this need a 2.x reroll?

Installing drupal/entity_browser (2.0.0-alpha2): Loading from cache
  - Applying patches for drupal/entity_browser
    https://www.drupal.org/files/issues/entity-browser-view-context-2865928-14.patch (2865928 - The View widget should filter based on field settings)
    https://www.drupal.org/files/issues/2877751-17.patch (2877751 - Inform users how many items they can add to a field that uses an entity browser)
    https://www.drupal.org/files/issues/entity-browser-file-widget-single.patch (Improve Field Widget UX for single-cardinality File/Image fields | https://www.drupal.org/node/2825555)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/entity-browser-file-widget-single.patch
    https://www.drupal.org/files/issues/2845037-4.patch (Fixed the issue of Call to a member function getConfigDependencyKey() on null | https://www.drupal.org/node/2845037)
    https://www.drupal.org/files/issues/entity-browser-quickedit-2733605.patch (Quick edit compatibility | https://www.drupal.org/node/2733605)

(From acquia/df, after switching to using acquia/lightning:dev-8.x-3.x)

What's left to get this in? Is there anything outstanding? It seems like a very simple, yet effective patch that is completely in line with the "future state" described by @dries in the keynote this week. We have been using this patch in front of prospects and customers on the Demo Framework distro for about a year and a half now.

Right now I have two patches both for this same view and it is an absolute nightmare getting composer patches to work in every scenario. We're ignoring patches and using a consolidated patch, but that doesn't always work. E.g. try making a composer project and including BLT + Lightning together with this patch. See #2877751: Inform users how many items they can add to a field that uses an entity browser as the failing/conflicting patch. Also proposed for the current sprint.

Perhaps you guys should just commit Sam's consolidated patch here: https://www.drupal.org/files/issues/2018-03-27/2877751-17_2825555-8_comb... and we can close both of these long standing, annoying issues that have been making my life hell with regard to composer trying to patch.

In short: Entity Browser needs these patches and it needs a new release. ✌️

Looks great, except for a couple of small things. Oh, and it needs tests :)

1. +++ b/src/Plugin/Field/FieldWidget/FileBrowserWidget.php
   @@ -209,38 +209,47 @@ class FileBrowserWidget extends EntityReferenceBrowserWidget {
   +    if ($cardinality === 1) {
   +      $current = [
   +        '#type' => 'container',
   +      ];
   +    }
   

   We should probably add a little help text in this case -- "No file selected" or something like that. Just to keep it consistent with the table.

2. +++ b/src/Plugin/Field/FieldWidget/FileBrowserWidget.php
   @@ -406,6 +415,7 @@ class FileBrowserWidget extends EntityReferenceBrowserWidget {
   +          '#access' => $cardinality > 1,
   

   This will cut off access to unlimited-cardinality fields. We should probably just check if $cardinality !== 1.

3. +++ b/src/Plugin/Field/FieldWidget/FileBrowserWidget.php
   @@ -421,11 +431,15 @@ class FileBrowserWidget extends EntityReferenceBrowserWidget {
   +    $ids = empty($values['target_id']) ? [] : explode(' ', trim($values['target_id']));
   

   Nit: for flexibility, we could change explode() to preg_split() by whitespace.

So I came across this issue whereby tabledrag.js was failing to re-order my files in the table. I've identified the issue (as discussed in this issue queue) with the #access key on the _weight element being incorrectly calculated.

I've added a patch that takes into account phenaproxima's comments.

I've left out point 1 because I personally can't see why this is needed (see my attached screenshot for why) - very open to being wrong btw! :)

Okay, so my colleague has pointed out I should have created the patch in a more sensible way! Ive also included point one as I didn't take into account the fact the cardinality might be 1.

:-)

Just a guess here, I wonder if the test is failing because of the hardcoded '1' in these lines of ImageFieldTest::: testImageFieldUsage() :

$this->getSession()->getPage()->fillField('field_image[current][1][meta][alt]', $alt_text);
$this->getSession()->getPage()->fillField('field_image[current][1][meta][title]', $title_text);

I think the '1' should be $this->image->id() maybe? That's only a total guess though. I don't know whether it's the node that's failed to load, or the image field list object, or even the first field item in it.

In terms of testing this change, I guess we need to ensure there's a test for a single-value image field, assert that the appropriate bits of text show before & after uploading the image to it and the multivalue field, as well as ensuring the ordering works for the multivalue field.

We could even go as far as adding a finite multivalue field to be tested in the same way too. Do we really want/need all those possible tests?

Here is a patch for the same issue but for the current stable version 1.4.

I think this might need a re-roll now that #2877751: Inform users how many items they can add to a field that uses an entity browser is in. Those two patches had always conflicted for us.

@brantwynn: I tried applying this patch to the HEAD of Entity Browser 8.x-2.x with git apply --check, and it seemed to work without complaint...

@SchnWalter's patch from #20 has been working great. Is there any reason this can't get committed?