Problem/Motivation

In terms of interaction, file fields could be a lot more accessible. The style guide proposes both a new appearance and a new interaction design.

Proposed resolution

11.file-field.png

  1. Files can be added from the local filesystem using drag-and-drop, or using the traditional click-browse-attach workflow (which hands the action off to the OS and back)
  2. Files upload automatically once added by dropping or browsing.
  3. Uploaded files provide a preview where possible (images) or a file-type-specific icon where not.
  4. File fields with multiple attachments use progressive disclosure to reveal each additional slot in turn. They should support dropping multiple files onto a single dropzone to upload multiple files at once.
  5. The “Browse Library” functionality is speculative at this time, but designed to accommodate contrib.

An important reason to encapsulate much of this functionality, is that it tends to scale much better amongst many items and especially if placed between forms it can serve as a clear visual landmark.

Aspects of proposed design postponed to follow-ups

Test Pages

  • A file field will have to be created and add to an existing content type

View all styleguide issues

Files: 
CommentFileSizeAuthor
#131 File field uploaded.png46.47 KBekl1773
#131 Field Field loading.png39.86 KBekl1773
#131 File Field hover.png40.67 KBekl1773
#131 Field field after.png40.79 KBekl1773
#131 File field before 8.3.1.png39.79 KBekl1773
#131 Screen Shot 2017-04-28 at 1.41.37 PM.png104.73 KBekl1773
#126 interdiff-12003609-124-126.txt5.35 KBkatzilla
#126 file_field_design_update-2113931-126.patch28.82 KBkatzilla
#124 interdiff-121-124.txt1.42 KBgaurav.kapoor
#124 file_field_design_update-2113931-124.patch27.21 KBgaurav.kapoor
#121 interdiff-2113931-116-121.txt8.77 KBDaniel_Rempe
#121 file_field_design_update-2113931-121.patch28.57 KBDaniel_Rempe
#119 Screen Shot 2017-02-21 at 2.27.07 PM.png174.12 KBtkoleary
#116 Image Multi settings for Article.jpeg79.3 KBAaronChristian
#116 Image Multi settings for Article Drupal.jpeg47.96 KBAaronChristian
#116 file_field_design_update-2113931-116.patch27.68 KBAaronChristian
#112 file_field_design_update-2113931-112.diff30.2 KBAaronChristian
#108 file_field_design_update-2113931-108.patch24.57 KBAaronChristian
#106 jquery-throbber.mov151.1 KBtkoleary
#106 Screen Shot 2017-02-09 at 11.58.07 AM.png233.51 KBtkoleary
#106 Screen Shot 2017-02-09 at 11.57.26 AM.png166.38 KBtkoleary
#104 interdiff-2113931-98-104.txt917 bytesDaniel_Rempe
#104 file_field_design_update-2113931-104.patch24.57 KBDaniel_Rempe
#101 interdiff-2113931-87-98.txt11.79 KBDaniel_Rempe
#99 multiple_files.png36.5 KBDaniel_Rempe
#99 multiple_images.png72.55 KBDaniel_Rempe
#99 files_upload.png14.47 KBDaniel_Rempe
#99 file_upload.png9.71 KBDaniel_Rempe
#98 file_field_design_update-2113931-98.patch24.65 KBDaniel_Rempe
#94 Image settings for Article | drupal 8.3.x 2017-01-23 12-53-17.png70.36 KBGábor Hojtsy
#94 Create Article | drupal 8.3.x 2017-01-23 12-55-07.png163.04 KBGábor Hojtsy
#94 Create Article | drupal 8.3.x 2017-01-23 12-51-40.png105 KBGábor Hojtsy
#87 2113931-87.patch20.36 KBdagmar
#87 interdiff-2113931-79-87.txt25.38 KBdagmar
#79 2113931-79.patch39.28 KBJo Fitzgerald
#72 interdiff-68-72.txt0 bytesjoelpittet
#72 2113931-72.patch20.76 KBjoelpittet
#69 FileField.mov76.42 KBGábor Hojtsy
#68 2113931-68.patch40.12 KBphenaproxima
#67 2113931-67.patch40.49 KBphenaproxima
#63 2113931-63.patch39.08 KBphenaproxima
#61 2113931-61.patch38.29 KBphenaproxima
#55 file_field_design_update-2113931-55.patch38.58 KBAaronChristian
#44 file_field_design_update-2113931-44.patch38.19 KBAaronChristian
#36 multi-value-image-field.png71.54 KByoroy
#29 interdiff.txt12.37 KBlauriii
#29 file_field_design_update-2113931-29.patch16.25 KBlauriii
#28 drop.gif164.35 KBjoelpittet
#27 file_field_design_update-2113931-27.patch12.23 KBlauriii
#10 seven-file-field-2113931-10.patch3.55 KBphilipz
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch seven-file-field-2113931-10.patch. Unable to apply patch. See the log in the details link for more information. View
#7 seven-file-field-2113931-6.patch68.87 KBLewisNyman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch seven-file-field-2113931-6.patch. Unable to apply patch. See the log in the details link for more information. View
11.file-field.png70.09 KBLewisNyman

Comments

LewisNyman’s picture

Issue tags: +Usability, +styleguide

Tags

swentel’s picture

nod_’s picture

Issue tags: +JavaScript

clearly missing a tag

klonos’s picture

...moving related issues to their dedicated metadata section.

philipz’s picture

Is there any plan to push this forward?
I wonder if there's an issue somewhere to bring the drop file functionality into core by some library or custom solution? This surely needs to be done before any styling :)

But maybe it is too much for the scope of this issue and we should focus only on styling the current file field implementation using those mockups as style quide?

LewisNyman’s picture

Title: File Field style update » File Field design update
Component: Seven theme » file.module
Issue tags: +Seven
Related issues: +#1683838: Add HTML5 Drag & Drop upload to Field file

@philipz Yeah we should tackle it all here. The style doesn't make sense without the functionality. Changed the title of the issue slightly to cover this. #1683838: Add HTML5 Drag & Drop upload to Field file was closed as a duplicate of this issue.

Changing the component to the file module but tagging with Seven.

LewisNyman’s picture

FileSize
68.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch seven-file-field-2113931-6.patch. Unable to apply patch. See the log in the details link for more information. View

I had a play around integrating the Dropzone library. The tricky thing with all these libraries it they all seem to want to control the upload process as well. It's nice but it makes it impossible to integrate with Drupal's AJAX system. I'm tempted to say the benefit of using an external library does not outweigh the cost of integrating.

Here's a patch anyway for the curious,

philipz’s picture

I'm trying to make it work with FileAPI only. The browser support seems OK but please let me know if this will be enough? FileAPI browser support.

LewisNyman’s picture

The support is fine, I'm not sure I get how FileAPI would help? We only want to add the interaction, we don't want to do anything with the file at all, just leave it up to the current behaviour.

philipz’s picture

FileSize
3.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch seven-file-field-2113931-10.patch. Unable to apply patch. See the log in the details link for more information. View

Yes I understand that we want to use current behaviour where possible. So the only thing we want to do is adding drag&drop and without using external libraries like dropzone.js.
What I have tested so far is adding a behaviour using only event.dataTransfer property for drag and drop events witch is a part of File API.
The patch does not work though - the files are not being uploaded.

LewisNyman’s picture

Yeah, this seems to be a common issue with libraries that don't handle upload themselves. The file field is not writable. That explains why most libraries handle upload themselves.

This means we either:

  1. Rewrite how the filefield AJAX is handled so it doesn't go through the form (sounds scary)
  2. Make the file field input invisible and stretch it over the 'dropzone' and let browsers handle the drag and drop behaviour. I can't find a list of which browsers/operating systems actually support this though.
philipz’s picture

I wonder if we could try and use parts of Drag & Drop Upload module here. Looks like it does it without external libraries and yet using standard Drupal file upload.

klonos’s picture

@philipz: I was about to post a link to that project but you beat me to it.

Bojhan’s picture

Issue tags: +frontend
Bojhan’s picture

Status: Active » Needs review
Bojhan’s picture

Assigned: Unassigned » nod_

@nod_ could you take al ook at it?

The last submitted patch, 7: seven-file-field-2113931-6.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: seven-file-field-2113931-10.patch, failed testing.

Dave Reid’s picture

Issue tags: +Media Initiative
Wim Leers’s picture

Assigned: nod_ » Unassigned
Issue tags: +DrupalCamp Ghent 2014

Discussed this with @LewisNyman and @G-raph at DrupalCamp Ghent. @G-raph is very interested in taking this on. But @LewisNyman pointed out that the biggest stumbling block is likely going to be the integration with Drupal's AJAX system-based file uploads. Hence I proposed to handle that side of things, if @G-raph could handle the CSS and general JS side of things. Basically, @G-raph would do everything except for the Drupal.ajax stuff.

Hopefully we can get this moving forward again :)

rootwork’s picture

Curious if this ever got moving again :)

nod_’s picture

Make it look right, I'll make it work right.

I think we can make the JS work fairly easily with the approach in #10.

Dave Reid’s picture

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

At this point I think this is 8.1.x material.

hass’s picture

Is this great new style available in a contrib module?

lauriii’s picture

Issue tags: -DrupalCamp Ghent 2014
lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Status: Needs work » Needs review
FileSize
12.23 KB

Now there should be working dropzone element with working add files button

joelpittet’s picture

Issue summary: View changes
FileSize
164.35 KB

Kinda sorta works:) Fun!

lauriii’s picture

Limited the functionality to file fields because image fields will be managed in #2115469: Image Field style update.

lauriii’s picture

One big questions I had was how are we going to integrate the multi field widget into this component because it has the drag handle. Anyone has ideas?

Status: Needs review » Needs work

The last submitted patch, 29: file_field_design_update-2113931-29.patch, failed testing.

lauriii’s picture

Wim Leers’s picture

Priority: Normal » Major

:O Great work, Lauri! This is a major usability improvement IMO.

yoroy’s picture

Yay @lauriii for working on this! Is your question in #30 about the code or the ui behaviour?

lauriii’s picture

@yoroy it is about the UI behaviour

yoroy’s picture

FileSize
71.54 KB

It would simply just work ;-)
Do you mean that there's no design for where the draggable thingy should be positioned?
That would need some design work indeed, current state is quite ugly:

lauriii’s picture

So there is a file component but it doesnt help much with the multi value field (with dragging). Just a notice that this issue is presumably mainly focusing on the file upload style instead of the image one which is again completely new story.

Bojhan’s picture

Lets move the [x] remove - next to the 4kb and then place the drag and drop at the far right.

AaronChristian’s picture

I've been working on this guy as an extension of lauriii's patch.

I'm having an issue installing uploadprogress.so for PHP 7.0, has anyone had any luck with this yet? I cannot find any guides on compiling the proper version. It seems like uploadprogress-1.0.3.1.tgz possibly is only supported up to 5.4?

droplet’s picture

Adding padding to file input is a clever way but we shouldn't do it. For example, IEs won't support it. Any browsers can take it off at anytime since it's not W3C standard.

DragEvent provided more info and more extendable. eg. we can prebuild thumbnails from client side.

AaronChristian’s picture

I've made some headway on getting this one towards a completed state. I just had a few notes I would love some feedback on;

1. The current design only accounts for an upload progression bar, however you need a 3rd party library (PECL upload progress or APC to be able to view this), in the majority of cases most users will be stuck with the existing throbber. Which currently, we don't have a UX pattern for.

2. Would it make sense to attempt to implement the HTML5 progress tag to replace the current upload progress structure of div's etc?

Thanks!

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.

lauriii’s picture

@AaronChristian: Please post your progress here so you can get incremental reviews and other people can keep working and testing it!

One of the ideas for the throbber design I had was to replace the icon on the left with a throbber.

AaronChristian’s picture

Hey guys, sorry this took so long. I was having some issues with the patch, so hopefully this has all the changes. If it doesn't work let me know and I'll revisit making it.

Thanks!

jhedstrom’s picture

+++ b/core/themes/seven/js/dropzone.js
--- a/core/themes/seven/seven.libraries.yml
+++ b/core/themes/seven/seven.libraries.yml

Rather than put this in the Seven theme, wouldn't it make more sense to be in file.libraries.yml?

Similarly, rather than add this to the existing widget, might it make more sense to either provide a new widget for files, or make this a configurable option on the existing widgets?

mparker17’s picture

@AaronChristian, I wrote a blog article on my best practices for generating patches and interdiffs, which might help you in the future: http://openconcept.ca/blog/mparker/best-practices-generating-patches-and...

AaronChristian’s picture

Awesome thanks @mparker17, will have a read.

Cheers!

Manjit.Singh’s picture

Status: Needs work » Needs review

triggering the testbot for #44.

Status: Needs review » Needs work

The last submitted patch, 44: file_field_design_update-2113931-44.patch, failed testing.

AaronChristian’s picture

Thanks Manjit.Singh, I'm at little confused as to what has failed in the test. Are you able to point me to what the issue in my code is? I've never really worked on submitting and fixing patches before.

Thank you!

AaronChristian’s picture

@jhedstrom thank you as well. I'll leave that to someone else to comment, I just worked off of what was there originally. However I do like the idea of a toggle setting or something.

mparker17’s picture

@AaronChristian to find out what failed:

  1. look in the comment where you submitted your patch
  2. under your patch, you'll see one or more buttons that show the version of PHP and MySQL used, and how many failures (in the case of the patch from comment #44, it looks like PHP 5.5 & MySQL 5.5 18,509 pass, 2 fail
  3. click on the button. You'll go to a page (in the case of the patch from comment #44, https://www.drupal.org/pift-ci-job/232917) that shows you which test classes and which assertions in those classes failed
Fidelix’s picture

If you just scroll through the patch contents, you'll see that the contents are mangled.

I bet it is good but the patch file itself is broken.

mparker17’s picture

@Fidelix that's actually intentional: binary files get included in the diff to make it easier to review (see #1111224: Introduce .gitattributes to end the CRLF/LF and binary diff horror, and detect/auto-fix whitespace errors, #2166013: Remove macros from .gitattributes to avoid Git errors., https://www.drupal.org/node/1054616#comment-4994354 for background).

(previously, binary files didn't show up in patches, and we had to manually download them from the issue queue and place them in the correct place in the repository before reviewing or committing).

If you're having trouble applying the patch, and you're working with a clone of Drupal 8.2.x core, try applying it with git apply. Modern versions of patch(1) should also work; but if you're patching on Windows, you may need to pass --binary (I have not verified this though — see the patch(1) manpage for more information).

AaronChristian’s picture

Not sure if this fixes things as my simple test on my dev server keep throwing 500 batch errors. But perhaps we can get this tested again.

colan’s picture

Status: Needs work » Needs review

This status change should help. :)

Status: Needs review » Needs work

The last submitted patch, 55: file_field_design_update-2113931-55.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Retagging for the media initiative's current tag.

Bojhan’s picture

Assigned: lauriii » Unassigned
phenaproxima’s picture

Found some stuff -- mostly coding standards violations, but also a couple of bigger concerns...

  1. +++ b/core/modules/file/file.module
    @@ -1275,6 +1277,29 @@ function template_preprocess_file_link(&$variables) {
    +/**
    + * Gets a class for the icon for a MIME type.
    + *
    + * @param string $mime_type
    + *   A MIME type.
    + *
    + * @return string
    + *   A class associated with the file.
    + */
    

    This doc comment appears to be completely inaccurate...

  2. +++ b/core/modules/file/file.module
    @@ -1275,6 +1277,29 @@ function template_preprocess_file_link(&$variables) {
    +function file_shorten_filename($link_text) {
    +
    +  $name = $link_text;
    

    There's an extra line of white space here, and it seems like the argument should be $filename, not $link_text.

  3. +++ b/core/modules/file/file.module
    @@ -1275,6 +1277,29 @@ function template_preprocess_file_link(&$variables) {
    +  $nameWoExt = chop($name, '.' . $ext);
    +
    +  $nameStart = substr($nameWoExt, 0, 10);
    +  $nameEnd = substr($nameWoExt, -4, 4);
    

    This may be nitpicking, but can the variables use a consistent case? Core favors $name_without_ext, not $nameWoExt.

  4. +++ b/core/modules/file/src/Element/ManagedFile.php
    @@ -238,7 +238,7 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
    +      '#attributes' => ['class' => ['upload-button js-hide']],
    

    This should probably an array with two elements: ['upload-button', 'js-hide']

  5. +++ b/core/themes/seven/css/components/dropzone.css
    @@ -0,0 +1,105 @@
    +.dropzone-buttons li {
    +  display: inline-block;
    +  margin-right: 10px;
    +}
    \ No newline at end of file
    

    Git needs a newline at the end of the file.

  6. +++ b/core/themes/seven/css/components/dropzone.css
    --- /dev/null
    +++ b/core/themes/seven/images/dropzone-new.png
    

    I wonder if these should be SVG, if possible, for flexibility's sake? (A side advantage is that the patch will not run afoul of Git's binary handling if it's using SVGs.)

  7. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +  };
    +  ¶
    

    The blank line has extra whitespace that needs to be removed.

  8. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +  if(Drupal.ajax) {
    +
    +    Drupal.Ajax.prototype.setProgressIndicatorThrobber = function () {
    

    Missing a space between if and (Drupal.ajax), and there's a needless blank line after it. It would also be nice if the setProgressIndicatorThrobber function had a doc comment to explain it.

  9. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      // move the throbber to the dropzone trigger
    

    s/move/Move

  10. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      //if($(this.element).hasClass('upload-button')) {
    +      //  console.log('uploaded');
    +      //} else if($(this.element).hasClass('remove-button')) {
    +      //  console.log('removed');
    +      //}
    

    Commented-out code blocks should be removed.

  11. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +
    +    Drupal.Ajax.prototype.success = function (response, status) {
    +
    +      // Remove the progress element.
    

    It seems incredibly dangerous to overwrite the prototype of the AJAX success function. Surely there's got to be a better-encapsulated way of doing this?

  12. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      // Save element's ancestors tree so if the element is removed from the dom
    

    s/dom/DOM

  13. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      // we can try to refocus one of its parents. Using addBack reverse the
    +      // result array, meaning that index 0 is the highest parent in the hierarchy
    

    Why use addBack() to reverse an array? If memory serves, ES5 has Array.prototype.reverse(). And if we're trying to find the nearest [data-drupal-selector] element, is there any reason not to use $.closest()?

  14. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      // If the focus hasn't be changed by the ajax commands, try to refocus the
    

    s/ajax/AJAX

  15. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      if($(this.element).hasClass('upload-button')) {
    

    Needs a space after the word if.

  16. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +        //console.log('uploaded');
    

    Commented-out debugging lines should be removed.

  17. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      } else if($(this.element).hasClass('remove-button')) {
    +        //console.log('removed');
    +      }
    

    This entire else if block can be removed.

  18. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      // move the throbber to the dropzone trigger
    

    s/move/Move

  19. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      if($(target).find('.image-preview').length) {
    

    Space after if.

  20. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +})(jQuery, Drupal);
    +
    +
    

    Two extra blank lines here.

  21. +++ b/core/themes/seven/templates/form-element--managed-file.html.twig
    @@ -0,0 +1,105 @@
    +{#{{ kint(children) }}#}
    

    Should this be here?

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
38.29 KB

Altered a few things in this patch (sorry for the lack of an interdiff, but I think binary data in the patch makes one impossible). And found a couple more things that give me pause...

  1. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      $dropzone.on('dragover mouseenter', function() {
    +        $(this).parents('.dropzone').find('.dropzone-trigger').addClass('is-hovering');
    +      });
    +
    +      $dropzone.on('dragleave mouseleave', function() {
    +        $(this).parents('.dropzone').find('.dropzone-trigger').removeClass('is-hovering');
    +      });
    

    This looks ripe to use $.on()'s event delegation...

  2. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,132 @@
    +      $('.js-upload-image').click(function(e) {
    +        $(this).closest('.js-form-item').find('input[type="file"]').click();
    +        e.preventDefault();
    +        e.stopPropagation();
    +      });
    

    A doc comment explaining this might be useful.

Status: Needs review » Needs work

The last submitted patch, 61: 2113931-61.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: +Needs subsystem maintainer review
FileSize
39.08 KB

Yoooo! This one oughta pass the tests.

I think a JavaScript maintainer should take a good long look at the JavaScript parts of this patch before it can be RTBCed.

Status: Needs review » Needs work

The last submitted patch, 63: 2113931-63.patch, failed testing.

The last submitted patch, 63: 2113931-63.patch, failed testing.

nod_’s picture

  1. +++ b/core/modules/file/file.module
    @@ -1286,6 +1288,31 @@ function template_preprocess_file_link(&$variables) {
    +  $ext = pathinfo($filename, PATHINFO_EXTENSION);
    +
    +  $name = basename($filename, '.' . $ext);
    

    Since we're on PHP 5 > 5.2 we can use:

    $name = pathinfo($filename, PATHINFO_FILENAME); 
    

    Not sure of the need for this though.

  2. +++ b/core/modules/file/src/Element/ManagedFile.php
    @@ -238,7 +238,7 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
    +      '#attributes' => ['class' => ['upload-button', 'js-hide']],
    
    @@ -255,6 +255,7 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
    +      '#attributes' => ['class' => ['remove-button']],
    
    +++ b/core/themes/seven/templates/form-element--managed-file.html.twig
    @@ -0,0 +1,105 @@
    +        <ul class="dropzone-buttons">
    
    +++ b/core/themes/seven/templates/image-widget.html.twig
    @@ -0,0 +1,27 @@
    +    <div class="image-preview">
    

    js-hide is special but the rest of classnames that js uses should be data attributes. as in data-drupal-*

  3. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,109 @@
    +    attach: function (context, settings) {
    ...
    +    attach: function (context, settings) {
    

    Missing the detach function

  4. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,109 @@
    +        $(this).parents('.dropzone').find('.dropzone-trigger').addClass('is-hovering');
    ...
    +        $(this).parents('.dropzone').find('.dropzone-trigger').removeClass('is-hovering');
    

    Why not closest?

  5. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,109 @@
    +      $(context).find('.js-upload-image').click(function(e) {
    

    always use .on(), delegation or not. and when triggering an event, always use .trigger() when dealing with jquery objects.

  6. +++ b/core/themes/seven/js/dropzone.js
    @@ -0,0 +1,109 @@
    +    Drupal.Ajax.prototype.setProgressIndicatorThrobber = function () {
    ...
    +    Drupal.Ajax.prototype.success = function (response, status) {
    

    Ok, we can't do that in a theme. Maybe we need to move things around in ajax.js so that it's easily overridable in themes

    As far as I understand this it to change the place where the indicator is appended. Almost seems like we should create a new progress indicator type.

    The lesser evil would be to go through Drupal.ajax.instances and replace the success and indicator callbacks for the file upload field only, not everyone.

Some other issues: when I add a file and remove it. The next time I try to upload a file I get the file selector dialog twice, some attach/detach thing not going on. Maybe because the attach behaviors are missing .once() after element selectors.

I wouldn't necessarily delegate the mousz*/drag* event, since It's more costly to run it on the whole form than just the zone we're interested in.

When biding/unbinding event always add a namespace. here it could be dragleave.file-upload mouseleave.file-upload or something.

There are a few debug lines that needs to be removed too.

It's close, great stuff :) the handling of the progress indicator need some work to be cleaner.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
40.49 KB

Thank you, @nod_, for the review! I didn't address any of your comments in this patch because a) I don't quite grok the JS stuff it's trying to do, and b) I just want the damn tests to pass :)

I hate it when I say "this will pass the tests", and it epically fails. So I'm not going to say this one will pass.

phenaproxima’s picture

Made a few changes to the JavaScript -- mostly trying to get rid of the dodgy method overrides in Drupal.Ajax.prototype. No interdiff due to binary data.

Gábor Hojtsy’s picture

FileSize
76.42 KB

Other than some spacing snafus (once a file is uploaded, the elements are too close to the borders of the file field), this looks pretty good. Attached some video proof :) The "hover" state for the file dropzone is also not styled as designed. Those are the only things I found visually.

Gábor Hojtsy’s picture

Also the progress bar and remove elements are not implemented exactly as designed, but there was no design for a file field with extra elements either.

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.

joelpittet’s picture

Tried to address the padding, made the button smaller(although the delete button could use a bunch more styling and a Seven preprocess hook)

Not sure how to deal with the preview image, it doesn't make sense to add it to the drop area because the size changes...

jonathanshaw’s picture

It looks like we've had the javascript maintainer review from @nod.

Overall it's very difficult to see from recent comments what work is still needed on this, hence needs IS update to clarify?

droplet’s picture

Just 2 cents.

I think it doesn't fulfill the IS and frontend code styles. We have to define a better HTML & CSS for it. e.g.:

1. Define a pattern.

Check it out the IS, we have 3 states: pre-upload -> uploading -> uploaded

<div class="dropzone">
    <div class="dropzone__preview">
        <!-- FILE FIELD -->
    </div>

    <div class="dropzone__container">
        <div class="dropzone__pre-upload">
            <div class="dropzone__actions">
                <button class="dropzone__add-files">Add File</button>
                <button class="dropzone__browse-libary">Browse Library</button>
                <button class="dropzone__other-button">Dropbox (Module Custom)</button>
            </div>
            <div class="dropzone__description"></div>
        </div>
        <div class="dropzone__progress is-uploading">
            <button class="dropzone__close">Close</button>
            <div class="dropzone__progress-filename">Uploading file-name</div>
            <div class="dropzone__progress-bar">
                <bar />
            </div>
            <div class="dropzone__progress-stat">Uploaded 2.6 of 4KB</div>
        </div>
        <div class="dropbox__uploaded">
            <div class="dropbox__uploaded-filename">interdiff--17.txt</div>
            <div class="dropbox__uploaded-stat">4KB</div>
            <button class="dropbox__uploaded-close">Close</button>
        </div>
    </div>
</div>

2. Sign off above pattern from frontend maintainers

3. Code & Add CSS3 Anime with basic JS

4. After real coding, sign off again from frontend maintainers, including the docs comments.

5. Sign off from UX/UE designers

6. Starting advanced JS work. (Pick a lib? Code our lib? Cross browsers testing and fixing..etc)

7. Sign off from JS maintainers

8. Happy Ending.

yoroy’s picture

Status: Needs review » Needs work
webchick’s picture

Issue tags: +sprint

This is coming up in the context of the Media library design at #2796001: [prototype] Create design for a Media Library. Adding to the current sprint; this looks incredibly close, and would be great to get it finished up for 8.3.

The last submitted patch, 72: 2113931-72.patch, failed testing.

jonathanshaw’s picture

Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
39.28 KB
tkoleary’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/seven/images/dropzone-new.png
    @@ -0,0 +1,219 @@
    +�PNG
    

    Looks like png code has been added here. There's two problems here; one, the image needs to be an SVG (for retina displays); and two, the image needs to be located in core/misc/icons in whichever directory matches it's hex color.

  2. +++ b/core/themes/seven/templates/form-element--managed-file.html.twig
    @@ -0,0 +1,105 @@
    +    <span class="field-prefix">{{ prefix }}</span>
    +  {% endif %}
    +  <div class="dropzone js-dropzone">
    +    <div class="dropzone-trigger"></div>
    +
    +    {{ children }}
    

    When I test this in the browser the image is loading into the same div as the upload button, not the area that contains the 'plus' image. I *think* the problem is in the nesting. Like the dropzone trigger div should wrap {{ children }}?

The rest of it is looking pretty good. I think it's getting close to done.

Bojhan’s picture

Very excited about this. Would this be novice changes?

katzilla’s picture

Assigned: Unassigned » katzilla
Gábor Hojtsy’s picture

Issue tags: +Berlin media sprint
katzilla’s picture

I have tested the latest patch against the media patch #151 here: https://www.drupal.org/node/2831274
as part of the Berlin media Sprint.

From what I see, it works already great with single file upload:


But for multiple file upload it still needs some work:
- the icon is missing for the uploaded files
- instead the icon is changed on the input for new file:


RajabNatshah’s picture

I like this improvement.

andypost’s picture

would be great to get this in 8.3
looks only svg and dropzone issues left

dagmar’s picture

Status: Needs work » Needs review
FileSize
25.38 KB
20.36 KB

Here is the image with the svg icons. I didn't find another example of an svg using sprites in core, should we pick one of the colors from the image to decide which folder use?

Also I found a small javascript console error when trying this patch (Uncaught TypeError: instance.element.hasClass is not a function), should be fixed now.

I couldn't fix #2 from #80.

vaplas’s picture

+++ b/core/modules/file/file.module
@@ -1286,6 +1288,31 @@ function template_preprocess_file_link(&$variables) {
+    return substr($name, 0, $limit) . '...' . substr($name, -4) . '.' . $ext;
$limit = 10;
$name = "drupalrocks" (11)
$result = "drupalrock...ocks" ?

#87: It seems this patch lost a couple files as compared with #79 :)

vaplas’s picture

And yet, substr not a good choice, because name can contain Unicode characters.
PS. How could I forget about it?! Because this is my first and forever light green issue on d.org :)

jonathanshaw’s picture

Status: Needs review » Needs work

NW for #88, #89 and #80.2

vaplas’s picture

What about this:

use Drupal\Component\Utility\Unicode;

function file_shorten_filename($filename, $limit = 10, $ellipsis = '…') {
  $limit = (int) $limit;
  if ($limit < 1) {
    # throw new \InvalidArgumentException('Dude, give me more space! Regards, file_shorten_filename');
  }
  else {
    $name = pathinfo($filename, PATHINFO_FILENAME);
    if (Unicode::strlen($name) > $limit) {
      $left_part = Unicode::substr($name, 0, floor($limit / 2));
      $right_part = Unicode::substr($name, -ceil($limit / 2));
      $ext = pathinfo($filename, PATHINFO_EXTENSION);
      $filename = $left_part . $ellipsis . $right_part . '.' . $ext;
    }
  }
  return $filename;
}

Tests:

$name = 'drupalrocks.png';
$limits = [42, 10, 3, 1, 0, -1, 'lol'];
foreach ($limits as $limit){
  $result["$limit"] = file_shorten_filename($name, $limit);
}

# Output $result:
array (
  42 => 'drupalrocks.png',
  10 => 'drupa…rocks.png',
  3 => 'd…ks.png',
  1 => '…s.png',
  0 => 'drupalrocks.png',
  -1 => 'drupalrocks.png',
  lol => 'drupalrocks.png',
)

Changes:

  • Control $limit
  • special ellipsis symbol '…'
  • Unicode functions strlen & substr
  • magic '4' gone, now it just middle place
dagmar’s picture

#87: It seems this patch lost a couple files as compared with #79 :)

What do you mean? The patch file size is significantly smaller because there is no PNG there.

vaplas’s picture

I'm mistake, sorry. Your patch great!

Gábor Hojtsy’s picture

Tested this again. The lack of padding in the single file scenario that I identified in #69 is still an issue. There was not design for image file uploads as to where the image thumbnail would be placed, so I think its fine for this patch to keep it in the box, but not without a padding :) Here is an up to date screenshot with the latest patch:

The multifile scenario does not look like at all as designed, the uploaded file does not show up as a small bordered item, but I think we can go step by step on this and ALSO the image variant of this was not designed and IMHO it would be sad to wait for that to happen for any improvement:

Where this is used as the default file widget (field settings), the Add files button is still displayed but of course does not work anymore. (The same button is removed when interacting with this widget on the entity form itself, so this does not appear there):

Finally the button says "Add files" even if the field is limited to one file. This problem applies to both the field settings and the entity edit form itself.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

katzilla’s picture

Assigned: Unassigned » katzilla

Working on this issue on global sprint weekend.

webflo’s picture

Daniel_Rempe’s picture

Status: Needs work » Needs review
FileSize
24.65 KB

Worked on this in pairprogramming with @katzilla. Current patch implements most parts of the given design. There are still some responsive issues in draggable table on multi value fields. The designed progressbar is not possible to implement at the moment due to a not functional implementation of uploadprogress for php7.

Daniel_Rempe’s picture

Added some screenshots to give an impression of the developent in #98.

Single file upload:
file upload

Multiple file upload:
files upload

Multiple images:
multiple images

Multiple files:
multiple files

jonathanshaw’s picture

An interdiff for #98 would help reviewers.

Daniel_Rempe’s picture

FileSize
11.79 KB

Here is the interdiff for #98.

jonathanshaw’s picture

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -181,11 +181,11 @@ public static function uploadAjaxCallback(&$form, FormStateInterface &$form_stat
+      $form[$current_file_count]['#attributes']['class'][] = 'ajax-new-content managed-file';

I think this should be:

$form[$current_file_count]['#attributes']['class'][] = 'ajax-new-content';
$form[$current_file_count]['#attributes']['class'][] = 'managed-file';
Daniel_Rempe’s picture

You are right. The classes should be implemented as array values. I am going to upload a modified patch tomorrow. Thanks for your review so far.

Daniel_Rempe’s picture

Here is the patch from #98 including the remarks from #102.

vaplas’s picture

FileSize
31.16 KB
247.83 KB
31.54 KB
15.48 KB
6.97 KB

Nice work! A couple additions to the test:

  1. inflexible resize after adding image (see resize.png)
  2. blinking when drags images, especially from top left corner (see blinks.gif)
  3. 'remove' label appears suddenly (see remove.gif)
  4. layout for image has gaps (see layout.png)
  5. we have two hidden ("Choose file", "Upload") and one vissible ("Add file") buttons, we need all it? (see buttons.png)
  6. and yet, a changes between #29 and #44 are not quite obvious). For example, the appearance file_shorten_filename(). We really need it now? (if yes, see #91)
tkoleary’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
166.38 KB
233.51 KB
151.1 KB

@Daniel_Rempe

This is coming together nicely. Just a couple of minor things I noticed.

1. The jquery ajax throbber should really appear *inside* the area where the drop icon is and not *push* the button and text over as it currently does.

See quicktime movie: https://www.drupal.org/files/issues/jquery-throbber.mov

On multiple file upload the asterisk indicating tabledrag has happened is reflowing oddly.


AaronChristian’s picture

Hey all, I have a patch that will fix the issues noted in comments #105 & #106.

Will post tomorrow morning once I get back into work.

Thanks!

AaronChristian’s picture

Here she be.

Should note: I've added RTL support as well.

AaronChristian’s picture

Status: Needs work » Needs review
vaplas’s picture

@AaronChristian, it seems this is a copy of #104 patch.

slashrsm’s picture

Status: Needs review » Needs work

Correct:

➜  /tmp  diff file_field_design_update-2113931-104.patch file_field_design_update-2113931-108.patch
➜  /tmp  
AaronChristian’s picture

Ahhhh crap, I attached the wrong patch file, not sure what happened there. This ones legit though.

AaronChristian’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 112: file_field_design_update-2113931-112.diff, failed testing.

AaronChristian’s picture

Ahh i believe this is failing due to the tabledrag changed (*) being moved within the handler instead of the cell (@tkoleary - fixes the reflowing of the asterik).

I'll have a deeper look into this.

AaronChristian’s picture

Okay think I found the issue.

One thing that still needs some attention is the default image settings page. That form has a bit of a different layout, wondering if we can force the same sort of layout as we have on a single image upload field.

AaronChristian’s picture

Status: Needs work » Needs review
tkoleary’s picture

@ AaronChristian

Lots of improvement here. As noted in UX meeting the only things I saw in simplytest were:

  1. The button only works first time, but the plus icon works all the time
  2. When the throbber appears it is on top of the icon, instead the icon should be hidden when the throbber is there.
  3. In narrower screens the description field overflows the parent containter
tkoleary’s picture

Issue summary: View changes
FileSize
174.12 KB

@AaronChristian

One more thing. When adding with multi value, for some reason it goes back to the other icon on the third upload. Weird.

katzilla’s picture

Issue tags: +DevDaysSeville
Daniel_Rempe’s picture

@katzilla and I worked on this a little bit more.
We removed the background image when the throbber is shown and realigned the alt and title input fields.

Status: Needs review » Needs work

The last submitted patch, 121: file_field_design_update-2113931-121.patch, failed testing.

tkoleary’s picture

@Daniel_Rempe

Looks much better.

Still have the wrong icon on multi-value fields though.

gaurav.kapoor’s picture

Status: Needs review » Needs work

The last submitted patch, 124: file_field_design_update-2113931-124.patch, failed testing.

katzilla’s picture

Fixed the coding standard issues and proposed a solution for the table on mobile. In some cases (when the display option of the file-field is activated, or when 'show row weights' is active on multi-value fields) there is too much content in the table, so it gets messy. We added an overflow, so that one can scroll the table.
We also fixed the issue with the wrong icon on multi-value-fields. Please test and provide feedback.

katzilla’s picture

Status: Needs work » Needs review
Daniel_Rempe’s picture

As we just discussed with @ifrik we should open a separate issue for all mobile display problems and try to get this in for 8.4 since this is already a long list of comments. We opened the issue here https://www.drupal.org/node/2863808

jonathanshaw’s picture

Review points made since #98 that have not been explcitly mentioned as having been addressed:
#116 default image settings page
#118.1

I've updated the IS to reflect
#98: "The designed progressbar is not possible to implement at the moment due to a not functional implementation of uploadprogress for php7." (and created a follow-up #2863846: File Field design update progress bar)
#128 "separate issue for all mobile display problems" #2863808: File Field design update mobile specific table issue

colan’s picture

If anyone's looking for something to work on at the DrupalCon sprint, I'd recommend this issue. Won't be there myself though. (I'd work on this myself, but I'm best kept away from anything front-end related.)

ekl1773’s picture

Here's a review of where things stand- I looked at a clean install of 8.4 for the "before" screenshot, then applied the most recent patch (#126) for the "after" screenshots. Further details below are outstanding issues from ~#106 onwards. Hope this is helpful!

Before:
before
After:
After
Hover:
Hover
Loading:
Loading
Uploaded:
Uploaded

#116, the remove button is next to the uploaded image thumbnail for the default image field settings page but not for a file upload field on an Add Content page. Also, the layout should be updated so the alt and title fields are *inside* the box with the uploader.
#118, throbber still appears next to icon on default image field settings page rather than on the icon

Also attch, this is what happens when you try to upload an image to an unlimited image field that already has a default image/file defined... this seems confusing, maybe the default image could be labeled? should it appear there?
image field screenshot

AaronChristian’s picture

Awesome thank you so much Elli. I'll review all your feedback as soon as I get back to my work place.

Safe travels home.

nithinkolekar’s picture

I can see alternative text is mandatory now , so could be possible to place filename as default alternative text ? this could save some time.

rootwork’s picture

@nithinkolekar Using the filename as alternative text is an antipattern. WordPress actually removed this functionality late last year for accessibility reasons.

Other references:

Bojhan’s picture

Did #123 get a fix? It looks like all other followups are filed.

@rootwork Thanks, that's correct - we won't do that and/or introduce that in this patch.

What is left for RTBC?

jonathanshaw’s picture

@Bojhan see #131

chr.fritsch’s picture

Assigned: katzilla » Unassigned