Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tkoleary’s picture

Issue summary: View changes

sized image

larowlan’s picture

Issue tags: +Novice

Tagging

ptkoroma’s picture

I do agree with this approach

reglogge’s picture

A couple of notes on that one:

- The first line "Upload an image to go with this article" comes from the "standard" install profile and therefore can be easily overridden by either a custom install profile or - even easier - in the configuration screen for each such field.
- The following lines of text can also simply be overridden by a site developer since they come from a theme() function. The relevant code for the function theme_file_upload_help() lives in core/modules/file/file.field.inc.
- The allowed file size, file types and number of files that can be uploaded are not fixed but can be changed through simple configuration. The llst of allowed file types for example could get quite long.
- There are additional configuration options for image fields relating to allowed pixel dimensions of images to be uploaded. These would also display when activated by a site developer.

Pulling all these in part dynamic strings together into one line could easily result in something rather long and ugly. I personally would keep everything as it is and leave it up to site developers to override this display if the want to on an individual basis.

tkoleary’s picture

@reglogge

I think our basic assumptions about how Drupal 8 should appear to a new user are not matching. From you post it seems that you look at a new install as a kind of template or raw material for what will become a CMS once a developer has spent some time configuring it.

The approach we started to take in Drupal 7 and continued in 8 is to provide the user with a CMS that just works out-of-the-box. With that in mind my suggestion would by the inverse approach to yours: give the user the most minimal and least obtrusive description and allow the developer to add to it if they find it necessary.

On your last comment about having things on one line, this could be remedied by setting each string to display inline-block so that they would wrap more naturally based on the content.

reglogge’s picture

Status: Needs review » Needs work

@tkoleary: Right you are. This is indeed a recap of the age old question whether Drupal is a framework or an application. I apparently am more on the framework side of things but can totally see your point.

Changing the texts is technically trivial (that's why you tagged the issue with "Novice") so the only question is: should we? I wouldn't, but then again, who am i to decide this :-)

Anyway, setting this to "Needs work" since there is no patch yet.

manningpete’s picture

Status: Needs work » Needs review
FileSize
2.55 KB

1. I emptied the default description "Upload an image to go with this article." from field.instance.node.article.field_image.yml (devs can still add description to that field later if/when they want)

2. I changed the order of the statements, so cardinality would appear first per the proposed example.

3. I removed the strong tags per the proposed example.

4. I simplified the language per the example.

5. I removed the br tag from the implode, and added a space (without the break tag, everything appears on the same line and no inline-block was required)

The last submitted patch, shorten-text-under-image-upload-2041629-6.patch, failed testing.

tkoleary’s picture

How about:

One file, 256 MB max, of type: png gif jpg or jpeg.

I know inserting the "or" before the last child in the types string is a challenge but it would be a nice touch. :)

mcrittenden’s picture

Status: Needs work » Needs review
FileSize
3.79 KB

Attempting to fix the tests.

CaptainWonky’s picture

Status: Needs review » Needs work
FileSize
15.97 KB

Manual testing on latest patch shows an extra dot after the size limit:

extra dot after size

mcrittenden’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

Whoops, that'd be my fault. Fixed here.

tkoleary’s picture

@mcrittenden

Looks terrific. This is a big improvement.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/file.field.inc
    @@ -449,20 +449,21 @@ function theme_file_upload_help($variables) {
    +    $descriptions[] = t('!size.', array('!size' => format_size($upload_validators['file_validate_size'][0]) . ' limit'));
    

    'limit' isn't part of the t() call, so it's not translatable.

  2. +++ b/core/modules/file/file.field.inc
    @@ -449,20 +449,21 @@ function theme_file_upload_help($variables) {
    +    $descriptions[] = t('Allowed types: !extensions.', array('!extensions' => check_plain($upload_validators['file_validate_extensions'][0])));
    

    Using index 0 here and above, shouldn't that be the index of the current item in the field?

superspring’s picture

Assigned: tkoleary » Unassigned
FileSize
3.8 KB

I've updated mcrittenden's patch to reflect the review in #13.
1. Limit is now translatable
2. According to the API, the 0th's index does not refer to field's index - see https://api.drupal.org/api/drupal/includes%21file.inc/function/file_vali... for more details

tkoleary’s picture

@Wim Leers, @superspring

I would like to mark this patch RTBC but I think Wim should validate whether it addresses his comments.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +quickfix

No interdiff was provided, so I'll assume that "limit" thing was the only change. In which case, RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, less verbosity! :) This is quite a bit longer patch than one might think at a glance.

Committed and pushed to 8.x. Thanks!

tkoleary’s picture

@superspring, @mcrittenden, @manningpete, @larowlan, @Wim Leers, @webchick, @CaptainWonky, @reglogge, @ptkoroma, Thank you all. This is a massive improvement.

Some may see this type of issue as a small thing but this is exactly the type of attention to detail in the authoring experience that adds up to a "delightful" experience for the user.

While we're at it here's another small change with big impact:#2018871: Make summary and details collapsed by default

:)

tkoleary’s picture

Issue summary: View changes

again

Status: Fixed » Closed (fixed)

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