Problem/Motivation

It seems that #3082694: Extra space between message and content when content has elements with margin-top didn't help everywhere I thought.

Proposed resolution

See before / after.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 interdiff.txt331 byteslauriii
#12 claro-better_messages_spacing-3086510-12--fixed-10.patch6.68 KBhuzooka
#10 interdiff.txt3.27 KBlauriii
#10 claro-better_messages_spacing-3086510-10.patch6.68 KBlauriii
#8 interdiff.txt5.9 KBlauriii
#8 claro-better_messages_spacing-3086510-8.patch6.79 KBlauriii
#3 Captura de Pantalla 2019-10-08 a les 15.24.51.png63.83 KBckrina
#2 Claro--messages--desktop--after.png904.2 KBhuzooka
#2 Claro--messages--mobile--after.png629.35 KBhuzooka
#2 Claro--messages--desktop--before.png866.63 KBhuzooka
#2 Claro--messages--mobile--before.png628.68 KBhuzooka
#2 claro-better_messages_spacing-3086510-2.patch1.19 KBhuzooka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Active » Needs review
FileSize
claro-better_messages_spacing-3086510-2.patch1.19 KB
Claro--messages--mobile--before.png628.68 KB
Claro--messages--desktop--before.png866.63 KB
Claro--messages--mobile--after.png629.35 KB
Claro--messages--desktop--after.png904.2 KB
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
CreditAttribution: ckrina at Lullabot commented
Status: Needs review » Reviewed & tested by the community
FileSize
Captura de Pantalla 2019-10-08 a les 15.24.51.png63.83 KB

LGTM! Both on a design and code review perspective.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/css/src/components/messages.css
@@ -21,6 +21,19 @@
+[data-drupal-messages],

    Could we add documentation on why this is needed? 

  2. +++ b/css/src/components/messages.css
@@ -21,6 +21,19 @@
+.field .messages-list,
+.form-wrapper .messages-list {
+  margin-bottom: 0;
+}

    Should we create a new inline variation of the message-list? We probably can't do that easily now so I'd be happy to move that to a follow-up.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Can we also update this to the design system?

saschaeggi’s picture

saschaeggi
he/him
CreditAttribution: saschaeggi at Unic commented
Status: Needs work » Reviewed & tested by the community

Updated the specs in the design system: https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system...

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work

Thanks for updating this to the design system!

#4 still needs to be addressed.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
FileSize
claro-better_messages_spacing-3086510-8.patch6.79 KB
interdiff.txt5.9 KB

Addressed #4.1. I think we should move #4.2 to future since it doesn't seem like there's a reliable way to determine when messages should be rendered inline. Maybe it's something we can work in a follow-up.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
CreditAttribution: fhaeberle commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs follow-up

This looks good! Tested it and setting the tag.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs review
FileSize
claro-better_messages_spacing-3086510-10.patch6.68 KB
interdiff.txt3.27 KB

I realized that the Drupal message library doesn't have dependency on jQuery. I removed it from here as well.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

The patch in #10 adds an extra trailing space to the end of the js/messages.es6.js.

Apart from the error above, this looks (more than) good for me :)

Do we really need the follow-up? (I assume that we don't need it.)

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
FileSize
claro-better_messages_spacing-3086510-12--fixed-10.patch6.68 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: -Needs follow-up

We should work on that when we find use cases for it. I don't think we need a follow-up.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Needs review » Reviewed & tested by the community

  • lauriii committed 5ca1c3e on 8.x-2.x 
    Issue #3086510 by huzooka, lauriii, ckrina, saschaeggi: Proposal for (...

  • lauriii committed 9078834 on 8.x-1.x 
    Issue #3086510 by huzooka, lauriii, ckrina, saschaeggi: Proposal for (...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed
FileSize
interdiff.txt331 bytes

Awesome! I had to run eslint --fix to make this comply with our coding standards. Interdiff attached.

Committed and pushed 🚀

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Updated issue credits.

Status: Fixed » Closed (fixed)

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