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.

Screen Shot 2013-05-03 at 9.58.22 PM.png

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

CommentFileSizeAuthor
#79 1986408-79-message-style-update-reroll.patch1.17 KBjoelpittet
#74 Screen Shot 2013-06-06 at 10.12.10 PM.png19.33 KBtim.plunkett
#73 messages-bugfix-1986408-72.patch1.16 KBry5n
#71 messages-style-1986408-71.patch3.68 KBry5n
#71 messages-71-rtl.png72.79 KBry5n
#71 messages-71-ltr.png70.86 KBry5n
#61 messages-style-1986408-61.patch1.63 KBry5n
#60 unbreak-messages-1986408-60.patch920 bytestim.plunkett
#52 messages.png61.38 KBtim.plunkett
#52 messages-1986408-52.patch418 bytestim.plunkett
#40 core-1986408-message-styling-40.patch12.56 KBLewisNyman
#28 ok-27.png6.01 KBechoz
#28 warning-views-27.png6.7 KBechoz
#28 error-27.png12.11 KBechoz
#28 error-multiple-multiline-27.png58.02 KBechoz
#27 core-1986408-message-styling-27.patch15.5 KBLewisNyman
#27 interdiff.txt10 KBLewisNyman
#25 multiple-errors-1em.png79.21 KByoroy
#25 multiple-errors-0538em.png331.4 KByoroy
#25 multiple-errors-multiline-0538em.png408.09 KByoroy
#23 warning-22.png9.49 KBechoz
#22 ok-22.png6.01 KBechoz
#22 views-warning-22.png6.43 KBechoz
#22 error-22.png12.18 KBechoz
#20 core-1986408-message-styling-20.patch7.08 KBLewisNyman
#19 ok.png6 KBechoz
#19 warning-views.png6.42 KBechoz
#19 error.png12.26 KBechoz
#18 core-1986408-message-styling-18.patch6.68 KBLewisNyman
#17 core-1986408-message-styling-17.patch6.82 KBLewisNyman
#15 Screen Shot 2013-05-06 at 10.44.27 AM.png29.33 KBBojhan
#15 Screen Shot 2013-05-06 at 10.45.17 AM.png15.23 KBBojhan
#15 Screen Shot 2013-05-06 at 10.45.50 AM.png15.72 KBBojhan
#11 1986408-messages-styling-11.patch15.16 KBoresh
#9 1986408-messages-styling-9.patch3.12 KBoresh
#4 1986408-messages-styling-4.patch3.11 KBoresh
#1 1986508-messages-styling-1.patch3.12 KBoresh
Screen Shot 2013-05-03 at 9.58.22 PM.png70.61 KBBojhan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oresh’s picture

I wish to remove div from selectors, but it will be overridden by system.theme.css - we should clean that too.
Icons minimized and added.

oresh’s picture

Status: Active » Needs review
echoz’s picture

Status: Needs review » Needs work

In 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.

oresh’s picture

Status: Needs work » Needs review
FileSize
3.11 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 1986408-messages-styling-4.patch, failed testing.

echoz’s picture

git error: corrupt patch at line 25
I suggest you test applying the patch before uploading, that should save us time and effort :-)

Bojhan’s picture

@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.

echoz’s picture

Thanks Bojhan, understood.

oresh’s picture

Bojhan, echoz
sorry guys - told you, I had lots of troubles with creating those patches.
I tried this one - it applied with no errors.

echoz’s picture

While 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).

oresh’s picture

@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 :)

oresh’s picture

Status: Needs work » Needs review
echoz’s picture

I'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.

echoz’s picture

Issue tags: +Needs manual testing

Found that elusive tag

Bojhan’s picture

I have been doing some manual testing, see the screens below. The images worked fine for me.

Screen Shot 2013-05-06 at 10.45.50 AM.png

Looks quite awesome :)

Screen Shot 2013-05-06 at 10.45.17 AM.png

Also looking awesome

Screen Shot 2013-05-06 at 10.44.27 AM.png

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.

yoroy’s picture

Status: Needs review » Needs work

1. 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.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
6.82 KB

Here's a bit of a rework.

  • Moved all styling changes into system, lets see what does and doesn't make sense across themes.
  • Changes markup and CSS to match new standards
  • Tweaked the horizontal placement of the success icon
  • Added vertical spacing between multiple errors and multiple message blocks
  • Added rtl styling
  • I was going to add the other pixel size varients of the icons in misc but then realised we are using a licensed font... we need to fix this before we can commit.
LewisNyman’s picture

Forgot to move the rtl styling back into the correct style sheet.

echoz’s picture

Status: Needs review » Needs work
FileSize
12.26 KB
6.42 KB
6 KB

I 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.

ok

Only local images are allowed.

error

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
7.08 KB

Sorry guys looks like my ul styling got lost somewhere

dbazuin’s picture

The 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.

echoz’s picture

Status: Needs review » Needs work
FileSize
12.18 KB
6.43 KB
6.01 KB

New 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.

ok-22.png

views-warning-22.png

error-22.png

echoz’s picture

FileSize
9.49 KB

Got a non-views warning screenshot:

warning-22.png

dbazuin’s picture

I 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.

yoroy’s picture

I 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

dbazuin’s picture

I think your right also every paragraph ends with a dot so thats also a clue that the are more messages.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
10 KB
15.5 KB

Turns 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.

echoz’s picture

Screenshots from #27 patch:

ok-27.png

warning-views-27.png

error-27.png

error-multiple-multiline-27.png

I still think bullets (circle) would make multiple messages clearer, but realize a bullet is not great next to the icon.

dbazuin’s picture

Thats 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.

ry5n’s picture

This 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! :)

yoroy’s picture

I'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.

Bojhan’s picture

The 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.

LewisNyman’s picture

What if we split each individual error into their own message box? Too much?

Bojhan’s picture

@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).

kika’s picture

To 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.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, sounds like we should go ahead then.

scronide’s picture

Could you not repeat the icon for each message without splitting the box? Use them as the bullets?

Bojhan’s picture

Status: Reviewed & tested by the community » Needs work

@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.

LewisNyman’s picture

How about committing without the icons and dealing with the icons in a followup? There's already an issue for general icon strategy right?

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
12.56 KB

Here'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.

Bojhan’s picture

Sure, lets do it. I am already getting the icons.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

RTBC!

Comitters please credit the designers:

Issue #1986408 by LewisNyman, oresh | Bojhan, r5yn, yoroy: Message style update.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

echoz’s picture

This did not get committed, oh fearless leader :-)

Bojhan’s picture

Status: Fixed » Reviewed & tested by the community

Hah, I was trying to figure that out too.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Oops! Committed to 8.x. And then committed it a third time to be extra sure. ;-)

nod_’s picture

Issue tags: +JavaScript

patch changes JS files, tagging.

Bojhan’s picture

@Dries Next time could you also credit the designers?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Fixed » Needs work

There 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.

ry5n’s picture

I’d like a chance to review the problems and proposed changes before they go in.

ry5n’s picture

If 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'.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
418 bytes
61.38 KB
+++ b/core/modules/system/system.theme-rtl.cssundefined
@@ -95,11 +95,21 @@ ul.menu {
+  background-position: 99.3% 19px;

+++ b/core/modules/system/system.theme.cssundefined
@@ -428,62 +428,67 @@ ul.tabs {
+  margin-top: .769em;

Stuff 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().
messages.png

ry5n’s picture

#52 Has .messages > ul > li + li {. This is not good. Read http://drupal.org/node/1887918

The new styles do need more thorough review though for mixed units, padding in px, etc. as pointed out.

tim.plunkett’s picture

I welcome another approach the preventing this cascade from breaking Krumo.

But we can't create bugs by blindly following "best practices".

scronide’s picture

Should 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?

ry5n’s picture

But we can't create bugs by blindly following "best practices".

I 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.

tim.plunkett’s picture

Krumo is third party, it's not devel module's CSS. This is absolutely a bug.

ry5n’s picture

@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.

ry5n’s picture

@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.

tim.plunkett’s picture

Krumo 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...

ry5n’s picture

@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.

tim.plunkett’s picture

Assigned: tim.plunkett » alexpott
Status: Needs review » Active

Actually, 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.

LewisNyman’s picture

Assigned: alexpott » Unassigned
Status: Active » Reviewed & tested by the community

This 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.

tim.plunkett’s picture

None of those added changes in #61 are in scope, but I just do not care any more.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

It pains me to invent a new tag for something like this, but visual testing is not enough.

ry5n’s picture

@tim.plunkett OK. After a thorough review, I found only one thing besides what was discussed and fixed in #49 to #61.

+++ b/core/modules/system/system.theme-rtl.cssundefined
@@ -95,11 +95,21 @@ ul.menu {
+  background-position-x: 99%;

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.

Bojhan’s picture

No 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.

LewisNyman’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Category: task » bug
Status: Needs review » Needs work

This 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.

ry5n’s picture

@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.

ry5n’s picture

Status: Needs work » Needs review
FileSize
70.86 KB
72.79 KB
3.68 KB

This 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.

tim.plunkett’s picture

+++ b/core/includes/theme.incundefined
@@ -1648,9 +1648,9 @@ function theme_status_messages($variables) {
-        $output .= '  <li>' . $message . "</li>\n";
+        $output .= '  <li class="messages__item">' . $message . "</li>\n";

+++ b/core/modules/system/system.theme.cssundefined
@@ -427,33 +427,48 @@ ul.tabs {
-.messages li + li {
-  margin-top: .769em;
+.messages__item + .messages__item {
+  margin-top: 0.769em;

I haven't manually tested, but I'm pretty sure these are the only lines that are in scope here.

+++ b/core/modules/system/system.theme.cssundefined
@@ -478,10 +495,13 @@ table tr.warning {
 .messages--error {
-  background-image: url(../../misc/message-16-error.png);
   border-color: #f9c9bf #f9c9bf #f9c9bf #e62600;  /* LTR */
 }
+.messages--error:before {
+  background-image: url(../../misc/message-16-error.png);

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.

ry5n’s picture

Ignore that last patch; I’m still being overzealous. New patch just fixes the actual bug.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
19.33 KB

Manually tested, and I don't think this patch could get much smaller or well scoped. Thanks @ry5n.
Screen Shot 2013-06-06 at 10.12.10 PM.png
You can compare to the screenshots in #52, this is correct again.

ry5n’s picture

@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.

echoz’s picture

@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.

ry5n’s picture

@echoz Let’s tackle those code improvements in a follow-up.

ry5n’s picture

Issue summary: View changes

meta issue added

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/messages-bugfix-1986408-72.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1184  100  1184    0     0   1553      0 --:--:-- --:--:-- --:--:--  2017
error: core/modules/system/system.theme.css: does not exist in index
joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

Here's the re-roll on #73

joelpittet’s picture

Issue tags: -Needs reroll

removing tag.

ry5n’s picture

Thanks Joel. Much appreciated.

Bojhan’s picture

Is this RTBC now? It looks like all concerns of tim are handeld?

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Yeah

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Bojhan’s picture

no commit message?

alexpott’s picture

Committed 95368fc and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.