Problem/Motivation

As noticed in comment #2505499-62: Explicitly support render arrays in hook_requirements(), for render arrays which have item lists as a title, the header padding is off when using drupal set message, causing the text to be misaligned with the icon. This is noticed in the seven (admin) theme.

Steps to reproduce

  1. Standard Drupal 8 Install
  2. Apply patch from comment #2
  3. Log into admin section of site created in step 1
  4. Visit "Extend" admin page (/admin/modules)
  5. Select Aggregator module
  6. Submit form by clicking on install button

The results of the above step should be an error message that looks like:

Proposed resolution

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Manually test the patch Novice Instructions
Add steps to reproduce the issue Novice Instructions X
Embed before and after screenshots in the issue summary Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 2550925-interdiff-14.txt341 bytesmaijs
#13 2550925-13.patch539 bytesmaijs
#12 2550925-12-after.png23.49 KBmaijs
#12 2550925-12-before.png23.6 KBmaijs
#11 2550925-11.patch539 bytesprineshazar
#6 2550925-6.patch1011 bytesprineshazar
#2 render-try-render-description-do-not-test.patch1011 byteslewisnyman

Comments

justAChris created an issue. See original summary.

lewisnyman’s picture

lewisnyman
he/him
commented
StatusFileSize
new render-try-render-description-do-not-test.patch1011 bytes

Uploading the patch you need to see this problem

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Active » Needs review
justachris’s picture

justachris commented
Issue summary: View changes
Status: Needs review » Active

No patch to resolve the issue has been submitted. Patch in #2 allows you to replicate the issue for testing.

prineshazar’s picture

prineshazar commented
Issue tags: +Barcelona2015

I'm taking a look (currently in First time sprint, room 117, DCBarcelona2015). I might roll a patch and get someone to review it.

prineshazar’s picture

prineshazar commented
Status: Active » Needs review
StatusFileSize
new 2550925-6.patch1011 bytes

Ive rerolled this patch

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented

I am mentoring at the DrupalCon Barcelona sprints, and have assisted PrineShazar through the patch re-roll process.

Status: Needs review » Needs work

The last submitted patch, 6: 2550925-6.patch, failed testing.

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented

The patch in #6 is intended to fail as it provides a method for reproducing the root issue.

The last submitted patch, 6: 2550925-6.patch, failed testing.

prineshazar’s picture

prineshazar commented
Status: Needs work » Needs review
StatusFileSize
new 2550925-11.patch539 bytes

Thank you opdavies. Uploading update to fix issue with misaligned heading when used via drupal set message (in this case a unordered list with title). I was not sure where to add header style changes, i inspected css file structure and the messages.css seemed like the most logical location to target header styles.

maijs’s picture

maijs commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2550925-12-before.png23.6 KB
new 2550925-12-after.png23.49 KB

With the patch from 2505499-86 (parent issue) applied, I applied the patches in #6 and #11. It looks good to me.

Before:
Before

After:
After

maijs’s picture

maijs commented
StatusFileSize
new 2550925-13.patch539 bytes 
+.messages h1, .messages .heading-a,
+.messages h2, .messages .heading-b,
+.messages h3, .messages .heading-c,
+.messages h4, .messages .heading-d,
+.messages h5, .messages .heading-e,
+.messages h6, .messages .heading-f {
+  margin-top:0;
+}

Updating the patch due to missing space between property and value.

maijs’s picture

maijs commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2550925-interdiff-14.txt341 bytes

Attaching interdiff to #13.

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented
Status: Needs review » Reviewed & tested by the community

I'm not sure if an interdiff was needed for such a small change, but it looks good to me anyway.

gints.erglis’s picture

gints.erglis commented

#13 patch can apply and looks good

jaxxed’s picture

jaxxed commented

@opdavies funnily enough I convinced maijs that an interdiff was necessary (best practices and all that), so #15 gave us a good laugh.

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented

@jaxxed: Here is the snippet from the Interdiff documentation page (https://www.drupal.org/documentation/git/interdiff). It doesn't hurt to upload one though. :)

You should supply one whenever you update a significant patch in the issue queues

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 79a1242 and pushed to 8.0.x. Thanks!

  • alexpott committed 79a1242 on 8.0.x 
    Issue #2550925 by PrineShazar, maijs, LewisNyman, opdavies, justAChris:...

Status: Fixed » Closed (fixed)

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