Originally submitted on Github

Problem/Motivation

Recent interviews and research exposed pain points around Drupal's admin experience of looking and feeling dated.

Proposed resolution

Implement new image field styles reusing existing components to create a favorable first impression of Drupal for evaluators and a better user experience for site authors. No functional differences.

Full Specs: https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system...

Remaining tasks

  • Update patch styling to include time inputs
  • Accessibility review
  • RTL review (Right to Left)

User interface changes

All image styles will be changed, no functional differences.

Test Pages

  • /node/add/article
  • /user/1/edit?destination=/admin/people
  • /admin/people/create
CommentFileSizeAuthor
#59 imageWidgetScreenshots.zip22.18 MBhuzooka
#59 imageWidgetScreenshots--high-contrast.zip2 MBhuzooka
#59 interdiff-3021092-47-59.txt2.48 KBhuzooka
#59 claro-image_widget_style_update-3021092-59.patch19.19 KBhuzooka
#58 Screen Shot 2019-10-11 at 17.05.56.png115.35 KBlauriii
#58 Screen Shot 2019-10-11 at 17.05.22.png118.89 KBlauriii
#49 Claro--image-widget--why-pattern-is-fixed.mp4986.52 KBhuzooka
#49 interdiff-3021092-39-47.txt4.66 KBhuzooka
#47 claro-image_widget_style_update-3021092-47.patch19.08 KBhuzooka
#43 Screen Shot 2019-10-10 at 20.31.49.png139.2 KBlauriii
#40 paddings.png142.53 KBckrina
#40 Captura de Pantalla 2019-10-10 a les 14.57.23.png20.09 KBckrina
#39 interdiff-3021092-37-39.txt489 byteshuzooka
#39 claro-image_widget_style_update-3021092-39.patch18.45 KBhuzooka
#37 claro-image_widget_style_update-3021092-37.patch18.43 KBhuzooka
#36 interdiff-3021092-34-36.txt16.04 KBhuzooka
#36 claro-image_widget_style_update-3021092-36.patch18.44 KBhuzooka
#34 Claro--image-widget--34-3.png1.97 MBhuzooka
#34 Claro--image-widget--34-2.png1.92 MBhuzooka
#34 Claro--image-widget--34-1.png2.54 MBhuzooka
#34 interdiff-3021092-28-34.txt1.69 KBhuzooka
#34 claro-image_widget_style_update-3021092-34.patch10.53 KBhuzooka
#32 image-file-upload.png127.68 KBant1
#28 interdiff-3021092-15-28.txt5.95 KBhuzooka
#28 claro-image_widget_style_update-3021092-28.patch10.06 KBhuzooka
#17 Claro--image-widget--15.png739 KBhuzooka
#15 claro-image_widget_style_update-302109-15.patch5.6 KBhuzooka
#11 Image-Widgets-in-Seven--annotated.png771.22 KBhuzooka
#11 Image-Widgets-in-Seven.png355.43 KBhuzooka
#13 image-file-column.png166.07 KBckrina
#11 Screenshot 2019-09-16 at 14.30.37.png240.3 KBhuzooka
#8 image-field.png174.14 KBckrina
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

antonellasevero’s picture

antonellasevero created an issue. See original summary.

antonellasevero’s picture

Issue summary: View changes
antonellasevero’s picture

Issue summary: View changes
ckrina’s picture

Version: » 8.x-1.x-dev
Status: Active » Postponed

Design isn't finished yet.

ckrina’s picture

Component: Code » Needs design
Issue summary: View changes
antonellasev’s picture

Issue tags: +beta blocker

ckrina’s picture

Component: Needs design » Code
Issue summary: View changes
Status: Postponed » Active
FileSize
174.14 KB

Uploading designs. This is ready to start.

ckrina’s picture

#3072660: Allow white transparent pngs to be seen in image-preview is related to this one and its suggestions are being integrated on the proposed design.

ckrina’s picture

Issue summary: View changes
huzooka’s picture

Issues I've found:

  1. Widget description missing from the designs. (This is missing from the file widget as well.)
  2. Details (and its spacing) does not follows the existing implementation (and varies).
  3. Table (the table heading) does not follows the existing implementation.
  4. No example for the case if title attribute input is (also) present.
  5. No example for the case if neither alt nor title are present. This case would look really weird with the current design imho.
  6. No example for the case if only a default picture is present (default image acts as a placeholder of an empty image field).
  7. It is still unclear how to display the preview of a portrait (or a landscape) image
  8. The design suggests that the image alt input (and i guess the title as well) should overflow from the table cell and use the space below the remove button, but this would be really weird.

    This can be achieved only by adding an another table row for the preview image and the alt (+title) input, but that would break the structure of the table (and would make the row rearrangement really-really difficult)

lauriii’s picture

Component: Code » Needs design
ckrina’s picture

Component: Needs design » Code
Issue summary: View changes
FileSize
166.07 KB

Adding the updated design fixing this. Please use Figma to check spacing other specs, but here's an quick screenshot:

huzooka’s picture

Assigned: Unassigned » huzooka
huzooka’s picture

Assigned: huzooka » Unassigned
Status: Active » Needs review
FileSize
5.6 KB

This is what we had previously (merged in the patches prior #20 in the File widget style update issue).

huzooka’s picture

Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

Attached a Chrome screenshot with patch #15.

ckrina’s picture

From this screenshot and implementing the patch locally I can see some things that need work that you might already seen since you're working on it:
- The error message is too far from the fieldset it belongs too. It's actually closer to the previous element in the form.
- The icon file is missing.
- "Limited image with preexisting value" variation: if there isn't the field to upload another image, there shouldn't be the last bottom border for the table. Sorry this wasn't defined, I think it's the first time anyone thinks about it :)
- The description/alt text are always below the thumbnail, although in the design when there's only one value it should be placed on the right. Anyway, with the goal to have a uniform design I'd keep the same behavior on both for now.
- The space reserved for thumbnail is not square as in the designs (even it's vertical or horizontal), but since we already have this follow-up it could be addressed there: #3072660: Allow white transparent pngs to be seen in image-preview.

huzooka’s picture

Re #18:

  1. The error message is too far…

    Yeah, this is not specific to this component, this is a general issue: #3082694: Extra space between message and content when content has elements with margin-top
    I guess we have to re-validate and change the spacing of the messages.

  2. The icon file is missing.

    Based on my last info, this should happen for image widgets.

  3. if there isn't the field to upload another image, there shouldn't be the last bottom border for the table.

    Honestly, why?

    I don't really like these kind of attitude: (i will exaggerate:) we define a component (table), and after that we have to introduce tons of variant for the component.

    I'll check whether this is possible with acceptable amount of extra code, but if it isn't, I won't do this.

  4. The description/alt text are always below the thumbnail, although in the design when there's only one value it should be placed on the right.

    I want to solve this if it wouldn't be a problem. (This is why this issue was in NW.)

  5. The space reserved for thumbnail is not square as in the designs

    And this is okay. The user needs to know and see the aspect ratio of the image that was uploaded. It would be a really basic usability issue if we would make every image look like a square PNG with alfa channel and transparent borders on the right and left side.

lauriii’s picture

Issue tags: +Needs followup

#19.1 That issue has been solved now but I don't think it fixes these. I don't think the message margins are aligned with other spacings and it looks off on most pages. Let's open a follow-up for that.

#18.2 I thought it was intentional decision to remove the icon. Was there a particular reason for the change of mind on this? If we indeed should do this, would it be possible to update this to the design system as well?

Honestly, why?

I don't really like these kind of attitude: (i will exaggerate:) we define a component (table), and after that we have to introduce tons of variant for the component.

I'll check whether this is possible with acceptable amount of extra code, but if it isn't, I won't do this.

Making judgments about other people like that hurts them. It's okay if you want to point out specific problems in the feedback, but you should assume the best intentions. I don't think @ckrina intentionally meant to cause trouble for us; I believe that she only tried to make sure the design looks good.

I agree that this isn't indicated by the design system and should be updated there as well.

#19.4 👍I think @ckrina pointed this out because #16 didn't indicate why this was moved to needs work.

#18.5 👍Let's discuss this in the follow-up.

lauriii’s picture

@ckrina: right, I totally forgot it!

@ckrina pointed out on Slack that she forgot that the icons were removed from the image widget. This addresses #18.2.

ckrina’s picture

Thanks @lauriii for the comment and +1 to all your points.

3. The design reason why the bottom line of the table is not needed is because its purpose is to indicate where its limits are or to separate it from other elements. None of this situations are needed here, and it's just adding visual noise.
That said, this isn't a big deal so if its implementation is complicated a follow-up or even not implementing it are both valid solutions. It just needed to be pointed out and discussed.

huzooka’s picture

Re #20, and @ckrina:
Please don't misunderstand what I wrote.
I raised my voice against the attitude, not against the person. I don't want to bee offensive. I just want to highlight this problem clear enough.

Re #22:
I don't agree. That line indicates where the table ends. Why do the default table needs those line and why this doesn't?
What if I remove that border? The bottom padding will be still there. The next step would be removeing the padding as well? And right after that we will notice that the dragging (and the dragged-last) color background looks a bit odd?

I still think that the default table styles are (more than) fine here as well, and wouldn't try to make an exception neither in terms of the UI design nor from CSS development.

lauriii’s picture

Re #23 & #20: I talked with @huzooka on Slack and we realized that there was some confusion in the translation. What @huzooka actually meant to say was that he disagrees with the approach, not the attitude. This makes much more sense and makes #20 sound much more reasonable 👍

huzooka’s picture

@lauriii just highlighted that... well... this is my english.

The right word I should have used is ATTITUDE, not approach APPROACH, not attitude.

Sorry for the misunderstanding.

ckrina’s picture

No problem at all! :)
I think we should just ignore this edge case for now and give it a thought with time on a design perspective to discuss this properly and not block the issue on this.
So let's leave the line there and finish this component that is almost done.

huzooka’s picture

huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
10.06 KB
5.95 KB

Review needed :)!

Notes:
Well, #3082694: Extra space between message and content when content has elements with margin-top not really helped us. (The approach is not the best as you will see even before and after applying this patch.)

ckrina’s picture

Assigned: Unassigned » huzooka
Status: Needs review » Needs work

On a design perspective it's perspective LGTM! 👍
Still needs proper code review.

huzooka’s picture

I just added a patch that fixes the (status) messages spacing: #3086510: Proposal for (status) message spacing..

ckrina’s picture

Status: Needs work » Needs review

Sorry I changed the status to Needs work. I meant to keep it as Needs Review to get another pair of eyes reviewing the code.

ant1’s picture

FileSize
127.68 KB

I don't know if this in scope of this issue but when a bigger preview image is set, the field next to the image become very small.

Image upload field

huzooka’s picture

Status: Needs review » Needs work

Re #32:
Well, yeah, I'd say this should be in the scope.

huzooka’s picture

This patch addresses the cases what #32 described:

If there is less space left next to the preview image than 60% of the parent div, the alt / title inputs are pushed to the bottom of the preview.

huzooka’s picture

Status: Needs review » Needs work
huzooka’s picture

Patch without rebase (and before applying the old-new standards).

huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
18.43 KB

...and now, the rebased one!

huzooka’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
huzooka’s picture

ckrina’s picture

Status: Needs review » Needs work
FileSize
20.09 KB
142.53 KB

I'm sorry I've found something I didn't see before: on small sizes there are no paddings on the fieldset when it's a multiple fields and it should keep the default ones defined for the fieldset component (smaller than the ones for desktop):

If there was only this feedback, I think it could be done in a follow-up.

huzooka’s picture

Re #40:

Yes, it's intentional. See point 1 of "Differences from the design" at #3021094-21: File field style update.
If you use Drupal from a handheld device, it saves some space for us.

(The screenshot you shared lies a bit, because on touch devices every button has the default size.)

huzooka’s picture

lauriii’s picture

  1. +++ b/css/src/components/file.css
    @@ -14,5 +14,9 @@
    -  padding-right: var(--space-l); /* LTR */
    +  padding-right: var(--space-l);
    

    🔬👁

  2. +++ b/css/src/components/image-preview.css
    @@ -0,0 +1,48 @@
    +  background-attachment: fixed;
    

    Why do we need this?

  3. +++ b/templates/content-edit/file-managed-file.html.twig
    @@ -36,11 +37,15 @@
    +    <div class="form-managed-file__meta-items-wrapper">
    +      <div class="form-managed-file__meta">
    +        <div class="form-managed-file__meta-items">
    

    🐎

  4. +++ b/templates/content-edit/image-widget.html.twig
    @@ -0,0 +1,61 @@
    + *
    + * Added by Claro:
    

    I would keep these as one list. I don't think we have usually documented where variables are coming from (I know we have one other instance which we probably could remove in this patch since we modify that file already).


  5. This might be something we should do in a follow-up, but the positioning of the description text is a bit off when the fields are shown next to the preview.
huzooka’s picture

Assigned: Unassigned » huzooka
ckrina’s picture

Assigned: huzooka » Unassigned

Re #41 I see the reason behind it and you're right, it's better to keep it like this to have more horizontal space for now, although my eyes hurt seeing it 😭.
I think we'll have to give it a thought on the design side in the future, but for now it's Ok.

ckrina’s picture

Assigned: Unassigned » huzooka
huzooka’s picture

Re #43:

  1. I think that this helps recognizing that the pattern behind the transparent images does not belongs to the image.

    [video]

  2. Reduced the number of the wrappers to 2. :)
  3. Done.
  4. Yeah. But we don't have to handle the current output imo: right after that we have the related FR #3084899: Make the description items of managed_file widgets distinguishable fixed in core, we will be able to follow the Figma design by displaying these descriptions above the widget (see #3021094-24: File field style update and #3021094-45: File field style update for the reasons why we don't do that).

    I created that (postponed) follow-up for us: #3087331: Unify the description display approach of single and multiple cardinality file and image widgets

Beside these ^^, I also found and fixed an another IE11 issue.

huzooka’s picture

Status: Needs review » Needs work
huzooka’s picture

huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review

ckrina’s picture

ckrina’s picture

The background pattern was first introduced at #3072660: Allow white transparent pngs to be seen in image-preview and incorporated here. Crediting who worked on that issue here.

saschaeggi’s picture

RE #47: I've created a follow-up design ticket to improve the pattern design #3087345: Improve transparent image background pattern usage

saschaeggi’s picture

ckrina’s picture

Status: Needs review » Needs work

Per Slack discussion, @lauriii @saschaeggi and myself we'd remove the fixed position from #43.2 for now and open the follow-up Sasch created to discuss its implementation.

lauriii’s picture

There's a regression on the file widget:

Before:

After:

huzooka’s picture

Patter fixing removed, #58 addressed, screenshots attached (Safari screenshots are incomplete because of this).

ckrina’s picture

Status: Needs review » Reviewed & tested by the community

LGTM!
Great work, and thanks for the patience with the changes @huzooka!

  • lauriii committed c41c2aa on 8.x-2.x
    Issue #3021092 by huzooka, ckrina, lauriii, AntoineH, antonellasevero,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

This is the final beta-blocker. Thank you everyone for the great work! 🙏

Committed and pushed! 🚀

  • lauriii committed b2a3ccf on 8.x-1.x
    Issue #3021092 by huzooka, ckrina, lauriii, AntoineH, antonellasevero,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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