More Seven Theme issues: #1986434: New visual style for Seven
Problem/Motivation
For Drupal 7 we did a massive overhaul to how our messages visually look, combining messages, simplifying the text and making it compliant with WCAG 2.0. We went a little overboard with that strategy, currently messages have a conflicting palette making them hard to read and often ignored. The proposed styling is more "smooth" calling for better integration with the color styles.
This style also includes new icons, sadly this is not within scope of this issue. We will need to update our icon library.
We developed Proposal: A Style Guide for Seven. This issue aims to introduce the proposed styling for messages in core
To quote the rationale provided:
Messages are proposed to follow closely from a style proposed for D7. This style attempts to make messages very noticeable (with a bright colour bar and icon(s) at the left margin) while preventing them from being visually overwhelming (avoiding a full border). Message colors are slightly modified from current values, both for contrast with white and to distinguish one from another.
The messages also propose simplified (one-color) versions of the existing icons and the theme-wide 2px border-radius.
Proposed resolution
In #1945554: Add Messages component we laid the ground work for this change.
We propose to add a small border to the left, reduce the color background and with that contrast. And increase some of the white space.
Remaining tasks
- Make a patch
- Add icons in separate issue
- Review accessibility
User interface changes
The message style will be changed, no functional differences.
API changes
None
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#79 | 1986408-79-message-style-update-reroll.patch | 1.17 KB | joelpittet |
#74 | Screen Shot 2013-06-06 at 10.12.10 PM.png | 19.33 KB | tim.plunkett |
#73 | messages-bugfix-1986408-72.patch | 1.16 KB | ry5n |
#71 | messages-style-1986408-71.patch | 3.68 KB | ry5n |
#71 | messages-71-rtl.png | 72.79 KB | ry5n |
Comments
Comment #1
oresh CreditAttribution: oresh commentedI wish to remove div from selectors, but it will be overridden by system.theme.css - we should clean that too.
Icons minimized and added.
Comment #2
oresh CreditAttribution: oresh commentedComment #3
echoz CreditAttribution: echoz commentedIn my test, the images didn't make it. They are in Seven's images directory, but don't show up and Preview suggests they "may be damaged". @oresh, is this not reproducible for you?
I believe the positioning for the background position (icon) should not be vertically centered, but vertically lined up with the first line where we might have multi lined messages.
Our hex color #s should be lowercase.
Comment #4
oresh CreditAttribution: oresh commentedI used to make patches so quickly and something happened with me at it takes 4 times more time to make the patch than the changes.
Icon aligned, should now be attached normally.
Comment #6
echoz CreditAttribution: echoz commentedgit error: corrupt patch at line 25
I suggest you test applying the patch before uploading, that should save us time and effort :-)
Comment #7
Bojhan CreditAttribution: Bojhan commented@echoz I never do that, and I bet loads of contributors don't. That's why we have a testbot, so we can quickly test and try patches - I generally don't try patches till testbot did a pass on it.
I think that this error because of a new line error.
Comment #8
echoz CreditAttribution: echoz commentedThanks Bojhan, understood.
Comment #9
oresh CreditAttribution: oresh commentedBojhan, echoz
sorry guys - told you, I had lots of troubles with creating those patches.
I tried this one - it applied with no errors.
Comment #10
echoz CreditAttribution: echoz commentedWhile this applies with no error, I get the same with the images as I did in #3. It does not appear in the browser in a message, nor when I drag one directly into a browser. Does this happen for you? They (the new pngs in Seven's image directory) will also not open in Preview (see #3).
Also more code review:
Hex color #s are back as uppercase.
No ending line break (helpful to set your text editor to do this automatically).
Comment #11
oresh CreditAttribution: oresh commented@echoz, hm..
i converted images through http://tinypng.org/ to make them way smaller. It could have killed them for no reason :)
The patch works for me adding the images and I can see them.
Adding the patch with uncompressed images. We can compress all the images later on in another issue, or maybe someone will succeed making the patch with working compressed images :)
Comment #12
oresh CreditAttribution: oresh commentedComment #13
echoz CreditAttribution: echoz commentedI'm getting the same thing with the images (so optimizing the images is likely not a problem). Someone else needs to test this, specifically the images. The css is rtbc for me.
Comment #14
echoz CreditAttribution: echoz commentedFound that elusive tag
Comment #15
Bojhan CreditAttribution: Bojhan commentedI have been doing some manual testing, see the screens below. The images worked fine for me.
Looks quite awesome :)
Also looking awesome
This I am not sure about, clearly we did not think about this part in the design. When you have several errors.
When it comes to the images though, feel free to exclude the icons from this patch - this needs overall style adjustment that I'd like to be split off from this issue. Also the icons included where not GPL, so we cannot introduce them.
Comment #16
yoroy CreditAttribution: yoroy commented1. Why the asterisk in the text of the warning message?
2. Icon alignment looks off, the one for error leans more to the left than the others. Or is this caused by different sizes screengrabs?
3. Yep, multiple errors in 1 box needs some additional work:
- have an (open) circle instead of a bullet for the list item?
- instead of bullets, seperate by lines between the items (probably too much)
- don't use bullets or seperators but rely on spacing, leave enough space between items to make clear there are multiple (is that accessible?)
- have a 'There are 4 errors:' above?
I remember we had a similar styling in d7 core for a while, with the thicker left border. Mark Boulton made an interesting comment about wether each status should be handled the same. Should a 'succes' message be as prominent as an 'error' message. This suggested using different widths for the left border depending on how critical the message is. Interesting but better in a follow-up I'd say.
Comment #17
LewisNyman CreditAttribution: LewisNyman commentedHere's a bit of a rework.
Comment #18
LewisNyman CreditAttribution: LewisNyman commentedForgot to move the rtl styling back into the correct style sheet.
Comment #19
echoz CreditAttribution: echoz commentedI think style changes for messages to be system wide vs just Seven would be fine, as these are nicer and better UX.
Some screenshots to review:
Views didn't pick up the .messages--warning class. This is the only place i could conjure up a warning.
Comment #20
LewisNyman CreditAttribution: LewisNyman commentedSorry guys looks like my
ul
styling got lost somewhereComment #21
dbazuin CreditAttribution: dbazuin commentedThe ok message looks fine.
The error I think is not right.
Beter keep the text in line with the ok message.
Of all the suggestions from yoroy I think the open circle is the best because it is more in style with the new look.
The warning png does not show in the latest png.
Comment #22
echoz CreditAttribution: echoz commentedNew screenshots:
Still, Views didn't pick up the .messages--warning class (sorry, incorrectly named shot in #19 was not displayed). Also, how can we trigger a help message?
I actually preferred the clarity of bullets in messages.
Comment #23
echoz CreditAttribution: echoz commentedGot a non-views warning screenshot:
Comment #24
dbazuin CreditAttribution: dbazuin commentedI think that increasing the whitespace between the paragraphs only works it there is at least one with more lines.
And thats not always the case like in your example.
So yes bullets but like yoroy said open ones or maybe just plain hyphen would be better.
Comment #25
yoroy CreditAttribution: yoroy commentedI tested with different values for margin-top. Latest patch seems to add a bit too much. I tried with 1em and 0.538em (latest patch: 1.538).
0.538em is *almost* enough, 1em is more than sufficient. Maybe there is a more exact measure in between these two values to be derived from the style guide?
To me the right amount of spacing seems enough, no bullets needed.
In Field UI, the 'unsaved changes' message had the same brownish look as shown in #22
Comment #26
dbazuin CreditAttribution: dbazuin commentedI think your right also every paragraph ends with a dot so thats also a clue that the are more messages.
Comment #27
LewisNyman CreditAttribution: LewisNyman commentedTurns out the message classes is added in other places in core... I've amended them with the new classes.
@yoroy - I've also changed the spacing between list items to 0.769em, this is half of the base line-height. At least it will be.
I've also tweaked the positioning of the icons.
Comment #28
echoz CreditAttribution: echoz commentedScreenshots from #27 patch:
I still think bullets (circle) would make multiple messages clearer, but realize a bullet is not great next to the icon.
Comment #29
dbazuin CreditAttribution: dbazuin commentedThats mainly because it is also a small circle. If someone could come up with a other (not a circle) error icon idea bullets are possible here.
One other thing that could help to make the bullets contrast more with the icon is keep the text in text color. so you get the border and background in the three colors but text just the basic text color.
Comment #30
ry5n CreditAttribution: ry5n commentedThis is a design flaw, although #28 doesn’t look too bad. In between other things I’ll see if I can think of anything else. Don’t wait up on me though! :)
Comment #31
yoroy CreditAttribution: yoroy commentedI've been hesitating to suggest a numbered list instead of bullets as another option. There's no real need to make it look pretty: multiple errors are no good, better to aim for clarity than subtlety.
Comment #32
Bojhan CreditAttribution: Bojhan commentedThe problem is people might start referencing to it, nor we can we do some type of indentation - as people might think its a structured error. Other than adding a landmark per error, I cannot see a reason to this more nicely.
Given that this patch works, and aligns with our restyling goal I suggest we get it into core.
Comment #33
LewisNyman CreditAttribution: LewisNyman commentedWhat if we split each individual error into their own message box? Too much?
Comment #34
Bojhan CreditAttribution: Bojhan commented@Lewis That would be too much, the whole reason we want to do it like this - is so multiple errors doesn't feel too overwhelming. In many cases multiple errors, are not that bad (e.g. to wrongly filled out fields).
Comment #35
kika CreditAttribution: kika commentedTo chime in, multiple message styling #28 is not 100% ideal but the best among the options suggested so far. Do not hold back because of it.
Comment #36
yoroy CreditAttribution: yoroy commentedThanks, sounds like we should go ahead then.
Comment #37
scronide CreditAttribution: scronide commentedCould you not repeat the icon for each message without splitting the box? Use them as the bullets?
Comment #38
Bojhan CreditAttribution: Bojhan commented@yoroy This cant be RTBC yet, the icons we are using here are not GPL.
@scronide We could but its just a little much, as a landmark. We don't want the message to shout.
Comment #39
LewisNyman CreditAttribution: LewisNyman commentedHow about committing without the icons and dealing with the icons in a followup? There's already an issue for general icon strategy right?
Comment #40
LewisNyman CreditAttribution: LewisNyman commentedHere's the patch from #27 with the icons reverted just because I'm around a laptop right now. This makes sense to me because we're touching a lot of core code with this patch, it's going to keep getting out of sync.
Comment #41
Bojhan CreditAttribution: Bojhan commentedSure, lets do it. I am already getting the icons.
Comment #42
Bojhan CreditAttribution: Bojhan commentedRTBC!
Comitters please credit the designers:
Comment #43
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #44
echoz CreditAttribution: echoz commentedThis did not get committed, oh fearless leader :-)
Comment #45
Bojhan CreditAttribution: Bojhan commentedHah, I was trying to figure that out too.
Comment #46
Dries CreditAttribution: Dries commentedOops! Committed to 8.x. And then committed it a third time to be extra sure. ;-)
Comment #47
nod_patch changes JS files, tagging.
Comment #48
Bojhan CreditAttribution: Bojhan commented@Dries Next time could you also credit the designers?
Comment #49
tim.plunkettThere will be another chance to give commit credit, as this needs a fix anyway.
The CSS styles are too generic, and they bleed into more complex output like Krumo.
I'll post a fix later today, and a suggested commit message to give credit where credit is due.
Comment #50
ry5n CreditAttribution: ry5n commentedI’d like a chance to review the problems and proposed changes before they go in.
Comment #51
ry5n CreditAttribution: ry5n commentedIf something else is using the class 'messages' the cleanest solution would be to simply change the name here to 'system-messages'. And also look at the other module to see if its CSS needs clean up. We can’t go back to selectors like 'div.messages ul li'.
BTW, this selector in the committed patch caught my eye: '.messages--error p.error'. That’s not great either. The 'error' class in itself is over-generic, the selector as a whole over-specific. Should be something like '.messages__message', then for message variants like error should be '.messages--error .messages__message'.
Comment #52
tim.plunkettStuff like this worries me. This patch seems like it had no code review, only visual. Why are we using such odd numbers and mixing em, percent, and pixel?
Ideally we would revisit this stuff, but mostly I want devel to not be broken:
Here is the problem. It is exacerbated with longer structures. The styles bleed through to any lists inside a d_s_m().
Comment #53
ry5n CreditAttribution: ry5n commented#52 Has
.messages > ul > li + li {
. This is not good. Read http://drupal.org/node/1887918The new styles do need more thorough review though for mixed units, padding in px, etc. as pointed out.
Comment #54
tim.plunkettI welcome another approach the preventing this cascade from breaking Krumo.
But we can't create bugs by blindly following "best practices".
Comment #55
scronide CreditAttribution: scronide commentedShould core be concerned with affecting contrib this way? Is it actually a bug? Shouldn't Devel address its rendering of Krumo inside the new message style?
Comment #56
ry5n CreditAttribution: ry5n commentedI totally agree. Best practices should help avoid problems exactly like this. If they don’t, they need revision. I don’t think that’s the case here though. I think a more specific name for the system messages component is all we need. I will review this in the next few days to flesh out my suggestions in #51.
Comment #57
tim.plunkettKrumo is third party, it's not devel module's CSS. This is absolutely a bug.
Comment #58
ry5n CreditAttribution: ry5n commented@scronide I agree, core should not write problematic CSS to work around contrib. This patch did nothing bad; the 'messages' class *would be* the right class name to use for this component. It’s appropriate for a unique (or primary) application-level UI component for message output, which is exactly what it’s for. However, the CSS standards are new, and if we can avoid immediate conflicts with major contrib modules, at least until they can be updated to the new standards, we probably should.
A bit of extra info: the new CSS standards say explicitly to avoid using generic class names like 'message' or 'messages' within a component. So if Krumo followed the new standards, it might use '.krumo__message'. Just like the main system messages might use .system-messages__message for child items, not .message.
Comment #59
ry5n CreditAttribution: ry5n commented@tim.plunkett RE: #57, I did not know that. In any case, I need to have a closer look at the situation. I’m sure there’s a solution that follows the new best practices and doesn’t interfere with Krumo.
Comment #60
tim.plunkettKrumo doesn't use 'messages'.
It doesn't use any specific classes.
It's just using <li>
But because it is printed inside a status message, it is inside the .messages.
That's the problem. ALL <li>'s in status messages will now get this margin.
Here's a new approach. I think the classname is bonkers, but that seems to be the trend...
Comment #61
ry5n CreditAttribution: ry5n commented@tim.plunkett I owe you an apology. I should have held my peace until I had the chance to look at the details. Instead, I jumped to all sorts of wrong conclusions. So yes indeed, this is a bug in the new stylesheet.
That said, #60 is on the right track, bonkers as it seems :)
So your analysis of the issue is right. We *could* do '.messages > ul > li', which limits the 'depth of applicability', leaving subsequent children untouched. But that starts to tie us too much to particular tags and DOM structure. There’s no absolute rule on this, but the cleanest way to get separation is to use single class selectors and basically namespace any child elements using the name of the parent component. We end up with longer class names but shorter selectors that are more resistant to changes in markup. They’re ugly, but they’re not bonkers.
Anyway, here’s my proposal to fix the issue. Note that I’ve removed the
.messages--error p.error
. After searching through core, it does not seem to affect anything not already covered by .messages--error and .error. I also removed the 'overflow-wrap property', it’s the newer version of 'word-wrap', but the spec states that the two must be treated as equivalent, and 'word-wrap' has broader support.Comment #62
tim.plunkettActually, based on the resulting bug this issue caused, as well as the numerous inconsistencies of em, pixel, and percentage based values IN THE SAME SELECTOR, I'd be much more comfortable with this being rolled back and actually receiving a code review, and not just a visual review.
Comment #63
LewisNyman CreditAttribution: LewisNyman commentedThis patch looks good to me. It prevents the reported issue and is inline with our CSS standards. The removal of the old p.error selector makes sense, it doesn't look like this is used in core.
By the way, there are design reasons for mixing units IN THE SAME SELECTOR. Some properties don't need to scale. For example, it makes no sense to increase the padding when the font size increased because you're reducing the line length when you actually need more. You would use em values to maintain vertical spacing based on line height.
Comment #64
tim.plunkettNone of those added changes in #61 are in scope, but I just do not care any more.
Comment #65
tim.plunkettIt pains me to invent a new tag for something like this, but visual testing is not enough.
Comment #66
ry5n CreditAttribution: ry5n commented@tim.plunkett OK. After a thorough review, I found only one thing besides what was discussed and fixed in #49 to #61.
Background-position-x isn’t standard and AFAIK isn’t supported in FF and Opera. It should use the full 'background-position' property. Note this still has to use % rather than px in RTL because of limitations in CSS. Ultimately a better way to do these icons is needed, but that’s a whole other kettle of fish.
I would support Lewis in #63. There’s nothing inherently problematic about mixed units in rulesets or even within a single property. The more important thing, say for example with padding values, is consistency across UI components, which I’m pretty sure we don’t have in core. If there is a standard that I’m not aware of, then this should be changed to match. Barring that, I would not call it a problem.
So I think all we need to do is fix the background-position-x, unless someone can correct me on it. Oh, and if you really feel like the p.error removal is wrong, we can leave it in and catch it in CSS cleanup.
Comment #67
Bojhan CreditAttribution: Bojhan commentedNo need to invent new tags. @lewis, r5yn please leave this at needs review for a while. So it can get feedback from others on the points @tim raised, there is no need to rush this in.
Comment #68
LewisNyman CreditAttribution: LewisNyman commentedComment #69
tim.plunkettThis is ridiculous. A CSS change was not manually tested, while our test coverage obviously cannot cover it. This issue introduced a bug.
The most recent patch has several changes that are completely out of scope.
If the patch in #60 is not sufficient, please post a properly scoped fix.
Comment #70
ry5n CreditAttribution: ry5n commented@Bojhan RE: #67 This has already been committed, and introduced a bug. We need to fix it.
@tim.plunkett:
1. I don’t know if this was or was not manually tested. Let’s let that be; I think we all agree that every one of these style updates must be manually tested. Including this fix.
2. I’ll roll a new patch that sticks strictly to fixing the bug and any code issues with the committed patch, #40. That includes #66. Hopefully such a patch will get this fixed without any more trouble.
Comment #71
ry5n CreditAttribution: ry5n commentedThis patch:
- Adds specific classes to the message elements, to avoid styles bleeding into message content (see #60)
- Uses generated content to display the icon, so that RTL doesn’t need to use inaccurate percentage values when moving the icon to the right.
- avoids setting border-width twice (was
border-width: 1px solid; border-width: 1px 1px 1px 8px
- consolidates two separate rulesets with the exact same style block (see patch line 127).
I’ve manually tested this in LTR and RTL in Chrome. Images attached.
Comment #72
tim.plunkettI haven't manually tested, but I'm pretty sure these are the only lines that are in scope here.
How is the relevant? I don't understand why any of this needs to change.
Please. Stop re-re-refreshing the message styling. Fix the bug that was introduced.
Comment #73
ry5n CreditAttribution: ry5n commentedIgnore that last patch; I’m still being overzealous. New patch just fixes the actual bug.
Comment #74
tim.plunkettManually tested, and I don't think this patch could get much smaller or well scoped. Thanks @ry5n.
You can compare to the screenshots in #52, this is correct again.
Comment #75
ry5n CreditAttribution: ry5n commented@tim.plunkett Thank you for your patience on this. The last half of this issue has been proverbially paved with my good intentions. No need to respond to this message; I just want to say I recognize my mistakes here and plan on learning from them.
Comment #76
echoz CreditAttribution: echoz commented@ry5n, I like your improvements in #71. That is a clever way to to get rid of the imprecise RTL percentage background-position. If we need to separate it from the bug discovered in #49 (I would be fine combining them), let's get it in a followup.
Comment #77
ry5n CreditAttribution: ry5n commented@echoz Let’s tackle those code improvements in a follow-up.
Comment #77.0
ry5n CreditAttribution: ry5n commentedmeta issue added
Comment #78
alexpottNeeds a reroll
Comment #79
joelpittetHere's the re-roll on #73
Comment #80
joelpittetremoving tag.
Comment #81
ry5n CreditAttribution: ry5n commentedThanks Joel. Much appreciated.
Comment #82
Bojhan CreditAttribution: Bojhan commentedIs this RTBC now? It looks like all concerns of tim are handeld?
Comment #83
aspilicious CreditAttribution: aspilicious commentedYeah
Comment #84
alexpottComment #85
Bojhan CreditAttribution: Bojhan commentedno commit message?
Comment #86
alexpottCommitted 95368fc and pushed to 8.x. Thanks!
Comment #87.0
(not verified) CreditAttribution: commentedUpdated issue summary.