#936304: [META] Style issue comments has recommendations for styling system messages.

The additional classes needed to identify system messages should be added in a drupalorg module preprocess function.


These type of messages usually correspond to the "colored" states in issue listing tables. It's a natural progression of quickly identifying automated messages in issues and should be themed differently from user provided content. Classes will need to be added to the .comment div to allow theming of different types:

  1. General .comment.system-message - This is a general style for system messages that do not have any particular correlation. Coloring matches the files fieldset background/buttons on issue edit.
  2. Testing Failed .comment.system-message.testing-failed - This matches the "Needs work" status in the issue listing tables.
  3. Testing Queued .comment.system-message.testing-queued - This matches the "Needs review" status in the issue listing tables.
  4. Committed .comment.system-message.committed - This matches the "Fixed" status in the issue listing tables. Generally speaking, there is usually only one commit per issue, so an issue is usually marked "Fixed" once a commit is made.
  5. Closed .comment.system-message.closed - This matches the "Closed (fixed)" status in the issue listing tables.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Assigned: Unassigned » drumm
drumm’s picture

FileSize
36.85 KB

Screenshot of the hover state from Mark Carver:

screenshot

drumm’s picture

Status: Active » Needs review

This is ready for initial review at https://drumm-drupal.redesign.devdrupal.org/node/2223537.

Notes:

  • The green for commits is a non-production color, it will match the mockups when compiled with the production variables.
  • Node changes styles are not updated anywhere yet.
  • files directory permissions on staging/dev are currently not fun, so user pictures are not generating. This is non-ideal, but not a problem.
Bojhan’s picture

It's a bit tricky to review without the real CSS colors. A few things I noticed:

- The hover on messages causes the text to wrap to the next line.
- The message #, how relevant is that? When do you refer to a test that didn't pass?
- The actual text is a little hard to scan, both because of the gray on gray and the actual terminology used.
- We probably shouldn't be using a special font size for these messages.

drumm’s picture

New URL to review: https://drumm2-drupal.redesign.devdrupal.org/node/2257197

All the greys are the real colors, so that is reviewable.

markhalliwell’s picture

  1. The reason the hover timestamp is causing the text wrap is because the original design doesn't expect long messages. In fact, we should customize the commit messages accordingly (I'll create a separate issue).
  2. I originally took this out, but had some blowback about the fact that a comment number "jumps". This would be evident in the above link where it would "jump" from #7 to #9.
  3. The colors used for foreground and background are fine: http://leaverou.github.io/contrast-ratio/#%23666-on-%23f6f6f2
  4. These messages are intended to be smaller (expect the permalink, which should match the normal comments for consistency), especially when dealing with long commit messages.
markhalliwell’s picture

Issue summary: View changes
markhalliwell’s picture

Issue summary: View changes
markhalliwell’s picture

@drumm, while working on #2275317: Cleanup commit message theme hook, I noticed that there are supposed to be <pre> tags around the commit message, however they are not currently showing up anywhere. I don't see any commits that have possibly stripped them out, however I do see field-name-comment-body is setup for multiple field-items (commits). Is the field somehow stripping these out?

Actually, it just dawned on me: System Message is posting using the Filtered HTML filter (which doesn't have <pre>). I'll create an issue to resolve this as well.

markhalliwell’s picture

tim.plunkett’s picture

DamienMcKenna’s picture

markhalliwell’s picture

drumm’s picture

I managed to lose the code for this in the DrupalCon commit shuffle. It is back now, ready for review, and <pre> is allowed.

A couple good examples are:
https://drumm2-drupal.redesign.devdrupal.org/node/2250429
https://drumm2-drupal.redesign.devdrupal.org/node/2228241 has multiple commits in the same comment.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
43.23 KB

Looks good. I have a few trivial comments that could be addressed as a follow-up (only if necessary):

  1. The comment links should not exist. The will never need to be reported for spam, edited or deleted :)
  2. The node changes table should probably be transparent and flushed with the message below it.

tim.plunkett’s picture

Actually, I delete system messages *all the time* in order to stay below the 300 comment threshold.
But, I could reshow that in a userstyle if its hidden with CSS.

drumm’s picture

If we remove the links, which I was going to do, but missed, the pages will still exist by direct URL. If we don't remove them here, or if we do and it isn't great, there's #2272951: Style comment links.

And #2272953: Style node changes in issue comments for the changes.

markhalliwell’s picture

Well, yet again, I'm not privy to such links and don't think about those kinds of things generally. Regardless it's very trivial and shouldn't block the majority of the styling that needs to make it on d.o. The issues @drumm mention should be sufficient to address them.

drumm’s picture

Over in drupalorg, committed the PHP that adds classes, etc: http://cgit.drupalcode.org/drupalorg/commit/?id=6a776ba

  • Commit 822338b on 7.x-1.x by drumm:
    #2211743 Style system message comments.
    
drumm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +needs drupal.org deployment

This is now on staging, will be deployed this afternoon.

drumm’s picture

Issue tags: -needs drupal.org deployment

Now deployed.

markhalliwell’s picture

Noticed a couple things:

  1. The new indicator needs to be aligned:
    Un-aligned:

    Aligned:

    CSS fix:

    #content .comment.system-message .new {
      margin: 5px 5px 0 -5px;
    }
    
  2. The commit message is sometimes too long, it's verbiage/styling needs to change slightly. I opened: #2290033: Hovering commit system messages can sometimes causes text to wrap/jump.

  • drumm committed 801b36b on 7.x-1.x
    #2211743 Improve spacing for new marker on system message comments.
    
drumm’s picture

Status: Needs work » Fixed
Issue tags: +needs drupal.org deployment

I implemented the CSS fix slightly differently, to avoid the negative margin.

drumm’s picture

Issue tags: -needs drupal.org deployment

Now deployed.

  • drumm committed 5fb75cf on 7.x-1.x
    #2211743 Add color for retesting.
    

Status: Fixed » Closed (fixed)

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