One of the directly visible changes in D7 is the removal of the teaser splitter and for this the teaser field is above the main content field.
This felt instantly awkward to me. I only saw two text fields and hesitated: "Ack, where do I enter my text now?". Like some strange kind of water pit that keeps me from starting. So the user must understand the teaser concept before he can write anything. not good.
In Wordpress, e.g. there is this field also, but collapsed and not even the next one after the main text field. I'd say 85% of the users will use automatically created teasers all the time.
Still: Any speculations from me are worthless, this must be user tested, and a decision will be easy.
How about creating a meta issues collecting untested UI changes in D7 to be tested? Vertical tabs belong there, too. The new overlay for admin stuff, the fact that the tabs are on the right side on the overlays.... lotsa stuff.
The user tests can easily be done in crowdsourcing and exposed on youtube. I am pretty sure if we have five or more Users tested we will quickly get a picture what is good and what has to be improved / changed. Definitely nothing that has not been tested should go in. Remember Teaser splitter desaster in D6.
Comment | File | Size | Author |
---|---|---|---|
#91 | 513414_drupal_text_summary_91.patch | 2.99 KB | sign |
#79 | node_edit_with_summary_after.png | 31.01 KB | sign |
#79 | node_edit_without_summary_after.png | 46.87 KB | sign |
#79 | node_add_expanded_summary_after.png | 28.59 KB | sign |
#79 | node_add_after.png | 6.54 KB | sign |
Comments
Comment #1
eigentor CreditAttribution: eigentor commentedComment #2
eigentor CreditAttribution: eigentor commentedComment #3
eigentor CreditAttribution: eigentor commentedAdded jet another tag for the issues that need testing.... Anyone slap me if they think tagitis is making things worse.
Comment #4
eigentor CreditAttribution: eigentor commentedComment #5
eigentor CreditAttribution: eigentor commentedComment #6
eigentor CreditAttribution: eigentor commentedHere is a first user test of the new position for the teaser field. Actually the user was testing the D7UX admin header, but the teaser field was also new for her, as she had never tried Drupal 7.
She has been working a while with Drupal 6 and is even creating an intranet project with it.
http://vimeo.com/groups/drupalux/videos/5657241
This points in a clear direction, but we need more user data to back it up. Have a look yourselves.
Comment #7
tstoecklerA new proposal off the top of my head:
Make the "Summary text field" a fieldset that is collapsed by default. That means less scrolling and less confusion for the 80% who don't need it and one click more for the 20% that do. I think that's a good tradeoff.
Comment #8
kika CreditAttribution: kika commentedAgree on summary/teaser field to be a collapsed field.
Comment #9
eigentor CreditAttribution: eigentor commentedI tried to create a patch for this, but it is just too hard for a non-programmer to find out where to add the fieldset. Is it text.module? node.pages.inc? Ack. Kika, can you do that?
Comment #10
yched CreditAttribution: yched commentedThat would be a change for text.module, text_textarea_with_summary_process()
Comment #11
eigentor CreditAttribution: eigentor commentedOK. Patches speak louder than words. If text.module keeps on tricking me, maybe some more user videos with them struggling with the summary water pit makes someone jump in :)
Comment #12
kika CreditAttribution: kika commentedSubscribing
Comment #13
kika CreditAttribution: kika commentedNo, I do not have such patch skills -- i just assembled an hack patch for eigentor for testing: it does not save summary value, just wraps the field into fieldset and get your test participants that lovely "collapsed-summary-feeling" ;)
It's ugly and wrapping a single form field into fieldset does not semantically make any sense but this is what we have in fAPI now.
Why oh why we do not have #collapsible / #collapsed property for _any_ form element?
Comment #14
kika CreditAttribution: kika commentedWhoopsie, now with less cruft
Comment #15
eigentor CreditAttribution: eigentor commentedComment #17
Bojhan CreditAttribution: Bojhan commentedThis is critical for Drupal 7.
Comment #18
catchI think what'd be quite nice is similar to what we have for machine names now. So a small bit of text which says ('summary, automatic'), when you click on it the text area is shown, you can't close it unless the field is empty.
Comment #19
stBorcherteigentor asked me to work on this so here is a first try. I tried to build it based on catch's idea (great idea, btw.).
Maybe we can use some ajax (as done in live) to get the filtered and trimmed version of the body text to display. At the moment there is no limitation on the teaser (display and field).
The first screenshot shows the initial state and the second one the state after clicking on "[Edit]".
Comment #20
tstoecklerEven though that is a gazillion times better than what we currently have, I still think that the summary kind of gets in your face too much. Especially when you write long texts, 90% of the users will see a lot of duplicated text, so I wouldn't mind making Summary a collapsed fieldset and/or moving it below the Body field (in addition to this greeat magic thingy!).
Comment #21
catchI just tried the patch and like it, a lot. However, after the initial cool factor, the text being duplicated up to starts to get a bit annoying (maybe having it below the body textarea where it wouldn't be immediately visible when typing would be less distracting).
Also it doesn't seem to actually trim the summary at the automatic length yet? I typed about four large paragraphs and it was still going.
Comment #22
stBorchertYes, its not trimmed actually.
Maybe we could load the trimmed version with AJAX? And apply the current selected filter if we are on it?
Comment #23
eigentor CreditAttribution: eigentor commentedThis is an interesting start :)
After applying the patch, at first I thought: hum, where is the teaser field gone?
Here is how it works: when you enter text in the main text field, there is a live preview of the teaser appering above the main text field labeled "summary".
It has some bugs: first, it always shows the full text, no matter how much you enter.
Secondly: once you clicked on edit, you cannot close it anymore. I see this may be what catch proposed.
This still has UX problems: I find it very confusing when the summary appears above my main text. As a novice user I believe I'd think "Wtf is this?"
What might help is if the summary was always hidden and only showed if I want to see it. So you have the label "Summary" or "Optional Summary" to click. It opens, you edit. Here a changing label "Automatic" if created automatically, or "Manual" if someone enters a summary different from the main text might help ( but might also confuse even more :( ).
Another challenge to be faced is to make clear just in the way it is displayed that this is a summary of your post that only appears on overview pages. Explanations and descriptions don't help. The D6 preview that shows teaser and full text I have seen to be very confusing for Users, they just don't understand the difference and why there are two texts.
Comment #24
Bojhan CreditAttribution: Bojhan commentedSo I think that the solution posted by stBorchert, has quite a few problems.
1. Preview of text is likely not going to be correct.
2. We will show text in a form, which is somewhat wierd.
3. Where do I go and edit this?
I was actually thinking of a far simpler solution.
1. Show and hide the summary.
2. Beign able to edit in one click.
However, one major disadvantage is not being able to see the teaser. But since, we can't really get that correct anyway - should we even try?
Comment #25
yched CreditAttribution: yched commentedThe current 'summary / full text' textareas are a minimal functional state to let the 'body as field' patch land. It can and definitely should IMO have its usability enhanced.
I kind of like Bojhan's proposal in #24.
- I'd rename the link from 'show summary' to 'edit summary' (we're in a form).
- Suggestion: I have a feeling that we could drop the 'textarea_with_summary' widget, and let this behavior be a widget setting for the 'text_textarea' widget, available only for for 'text_with_summary' field type.
Comment #26
catchI like both of yched's suggestions, don't necessarily have to be mutually exclusive.
Comment #27
stBorchertOk, here is a new one. I've tried to implement Bojhan's proposal.
Comment #28
Bojhan CreditAttribution: Bojhan commentedSo the current behavior should be changed.
1. The edit summary, should always be visible even, when nothing has been entered.
2. It doesn't trim it? catch told me it was going to be hard to trim it.
3. Please make it (Edit Summary) and make it the same size as the full text.
4. Do we have a WYSIWYG to test how it looks?
For the rest it looks good.
Comment #29
stBorchertNext try.
Done.
Yes. We need to add some AJAX magic to get the trimmed version of the body.
Done.
Uhm, (AFAIK) no.
Comment #30
yched CreditAttribution: yched commentedNote that whatever solution we end up with regarding my 2nd remark in #25, it needs to apply to all 'text fields with summary' widgets, not just the specific body field on nodes.
Comment #31
eigentor CreditAttribution: eigentor commentedAha, this is getting closer :)
Actually, I like the fact that the summary field is empty when you open it. For if it shows the trimmed text this might not be a great help to the user. If someone wants to use it, he has to change it anyway and might as well copy over a portion of the main text if all he wants is to control the length of the teaser.
Still, any way we do it it needs to be User re-tested. Maybe having two or three versions of it and doint a/b testing will tell.
I am still wondering if it does make sense to make this feature so prominent for it is very daunting to click the link "edit summary" and then not understand what it is about. So an alternative would be to position the Link below the main text field, this would mean below the format chooser. But very probably this would not help much and cause even more "Wtf is this?"
So the challenge is still making visually clear what this is about. The moment the user opens the teaser field he must understand what this is doing.
And alas, I cannot think of a way to do this in the form. So maybe a big "What is a summary?" link or the "more help" link, that leads to a help popup that shows something like this:
Help texts suck, but what do do. At least if the help is good :)
Comment #32
tic2000 CreditAttribution: tic2000 commentedText with summary as it is now in HEAD has one big advantage IMHO. It does the trimming on display, not on node save/edit and gives you the possibility to change the desired length at a later time and all your nodes (old ones too) will use the new length. The text is not trimmed if a summary is added.
Adding the trimmed text in the summary textarea will make it "permanent" and brake this feature. If you decide your summaries are too long/short and want to "resize" it will be an annoying task for older nodes because even an update will not touch the summary, so you have to go and edit manually all your old nodes. You can't do something really automatic because you don't know if the summary is a real summary or just a trimmed version of the text.
Given the above you can do all the usability changes desired (and I agree with them), but let the user fill the summary if he desires, don't do that for him.
Comment #33
stBorchertNext patch without filling the summary automatically.
Comment #34
stella CreditAttribution: stella commentedVisual review of patch:
if ($('#edit-body-0-summary').val() == '')
) - you should use===
here (see "Operators" section on JavaScript Coding Standards).body-0
. I admit I haven't looked into it, so someone correct me if I'm wrong, but are we guaranteed that it'll always have0
in the class / id names?I've tested it out and it works great, and degrades gracefully too :)
Comment #35
stBorchert@stella
* Line 41 ... ok, shouldn't be that hard to fix
*
body-0
... yeah, I know. It is indeedbody-0
every time but we can have multiple fields with teaser and I don't know how to process all of them. Maybe I'll have to take all "summary-wrapper"s and act on its neighbours. It would be great if a js-guru could have a look on it.Comment #36
stella CreditAttribution: stella commentedCan we have more than one field with a teaser? I haven't been following fields-in-core closely, but where would these additional teasers be displayed? I suppose they could be used in views...
I don't have time to look at this now, but will take a stab at it this evening.
Comment #37
stBorchertUhm, I thought we could have. But as I think about it, it wouldn't make sense to have more than one summary.
Or am I totally wrong?
Comment #38
tic2000 CreditAttribution: tic2000 commented@stella
In theory you can have more text with summary fields, but they won't be named body.
Comment #39
catch"Maybe I'll have to take all "summary-wrapper"s and act on its neighbours."
That sounds sensible since we can't guarantee what the field will be called. The body field is now only an instance of 'text with summary', so a new site could create all their nodes with a 'main content' field (or attach this field to users to use for profiles, or whatever), and ideally it should work the same for all.
On that note, the javascript should probably go in modules/field/modules/text/text.js rather than node.js
Comment #40
yched CreditAttribution: yched commented[edit: crosspost and almost exact duplicate with catch :-p]
You can definitely have several 'text with summary' fields on a node or an any other fieldable object. The 'body' field is only one example of such a field, and it won't be there on non-nodes, and even not on all nodes (node types can choose not to have a body field, but other 'text with summary' fields instead).
That's why the current patch is OK while the behavior is being fine tuned the behavior, but as I said a earlier, the final patch can absolutely not be hardcoded on 'body'.
The js script should be provided by text.module, not node.module. The textarea_with_summary widget should add a specific class to the HTML it generates, and include the script. The script then adds the behavior to all the elements in the form with that class. Very similar to "add the 'collapsible fieldset' behaviour to all fieldsets marked collapsible". Here we want to add the 'collapsed summary' behavior to all textarea_with_summary form elements.
Comment #41
Bojhan CreditAttribution: Bojhan commentedSo we have largely agreed on the behavior, I am thinking some text changes - but the rest is fine.
Comment #42
stella CreditAttribution: stella commentedbehaviour yes, but not the code - it needs to be able to handle more than one textarea. Meant to look at that earlier in the week, will do so now.
Comment #43
stella CreditAttribution: stella commentedOk here's a new patch. To summarise what it does:
Comment #44
stella CreditAttribution: stella commentedComment #45
yched CreditAttribution: yched commentedThe approach is looking great, thanks stella.
Having "Body" in the name is wrong ;-)
I'll leave a more insightful review to more JS-savvy people, but it sure looks like all those
$(this).closest('fieldset')
lookups should be optimized : searched once, saved in a variable, not searched again...This review is powered by Dreditor.
Comment #46
stella CreditAttribution: stella commentedOk here's a re-rolled version which addresses those 2 items.
Comment #47
yched CreditAttribution: yched commentedI forgot: instead of
it's preferable to use
as an entry in the $element array (better cacheability)
Comment #48
Bojhan CreditAttribution: Bojhan commentedYou mean this?
Comment #50
Bojhan CreditAttribution: Bojhan commentedOk, fixing tests - and array position.
Comment #52
stella CreditAttribution: stella commentedre-rolled patch using #attached_js
Comment #53
catchComment #54
stBorchertThis looks really great. One minor note: the grippie isn't expanded to the full width of the summary textarea anymore (see screenshot).
Apart from that: RTBC.
Comment #55
Bojhan CreditAttribution: Bojhan commentedFixing the title. Would love if a field expert could give this a last review.
Comment #56
stella CreditAttribution: stella commented@stBorchert - hmmm the grippie issue you identified doesn't happen on my drupal head installation. I've tested in FF3.5 (Mac), FF3.0 (Windows), Safari 4 (Mac) and IE8. What browser(s) does it happen in for you? Does it happen even when the patch isn't applied?
Comment #57
stBorchertBefore applying the patch the grippie expands to the full width of the textarea.
Tested on a fresh install (updated to latest head) with FF3.5 and Safari 4.0.2 (both Mac).
The value for
grippie.style.marginRight
isn't calculated correct because the element isn't visible (see textarea.js line 19).grippie.offsetWidth
is0
if its hidden just like$(this)[0].offsetWidth
.Comment #58
stella CreditAttribution: stella commentedChanging the order that the javascript is attached to the page would fix the issue. I was able to reproduce this, and was able to fix it via firebug by changing the execution order of the scripts. If textarea.js is loaded before text.js then the problem should disappear - but I'm not sure of the best way to force that to happen - maybe by adding textarea.js to the #attached_js before text.js? what do others think?
Comment #59
sunWhy reversed? The class name should match the behavior name.
Additionally, :not(.textarea-summary) is missing in the selector and .addClass(.textarea-summary) should come right before the .each().
Variables that are jQuery objects should be prefixed with $.
Why type-agnostic?
If you would have used $-prefixed variable names, you would know that $div is already a jQuery object. This double-initialization of jQuery continues in the rest of behavior.
Always use .length instead of .size().
(and elsewhere) Most of this can be chained; use .end() to continue with a new .find() on the original object.
I'm on crack. Are you, too?
Comment #60
stella CreditAttribution: stella commentedHere's a patch which addresses the items raised by sun, except for
Additionally, :not(.textarea-summary) is missing in the selector and .addClass(.textarea-summary) should come right before the .each().
. As I discussed with sun on IRC, the class is added by the php code and so does not need to be added here. Instead we're now adding atextarea-summary-processed
class instead to prevent it being processed more than once.Comment #61
sunThe context argument is missing here. The attach function should also have the new 'settings' argument for consistency.
.addClass() should always be invoked as early as possible, i.e. directly before .each().
Because: Unlike PHP, JavaScript is asynchronous. There is nothing that prevents this behavior from being invoked again while the previous invocation did not complete.
These classes seem to belong to the same logic, but their names and namespaces are very different.
Additionally, all of this seems to belong to text.module/text.js. That means, the proper namespace/prefix for all those CSS classes is "text-".
This review is powered by Dreditor.
Comment #62
stella CreditAttribution: stella commentedPatch re-roll.
Comment #63
sunSorry for not catching this earlier: CSSDoc follows PHPDoc to almost 100%. I.e. the summary should be on one line, no double spaces, and it looks like the current text is an explanation, so we need a new, tight summary.
Thanks for adding context!
Now we just have to use it as second argument to $() :)
14 days to code freeze. Better review yourself.
Comment #64
stella CreditAttribution: stella commentedDamn, sorry for missing that. Patch re-roll.
Comment #65
Bojhan CreditAttribution: Bojhan commentedIf tha_sun is oke with the last patch, I will mark this RTBC. Just tested it and still works as expected, would be nice to get this in before the CCK UI finetweaking begins.
Comment #66
sunSorry, I changed my mind about .end() in this case, because here, it makes the code flow very hard to read.
Attached patch needs manual testing once more. If it works, RTBC.
Comment #67
yched CreditAttribution: yched commentedMinor point : sun definitely knows JS and jQuery conventions much better than I do, but i'm a bit suprised by the number of .find() calls in there, while there's only 8 occurrences in the whole of our other .js files (jq and jq_ui source files excluded)
Don't we usually favor the $(selector, parent) syntax ?
Comment #68
Bojhan CreditAttribution: Bojhan commentedYup, working - just tested. - Oops didnt see yched comment.
Comment #69
sunComment #70
sunFirst pass.
Though on a side note: Oh dear. I'm entirely not happy with this widget and the underlying implementation.
One text format for 2 input fields. That's no better than the ugly teaser splitter we had in D6. Proper client-side editor support (Wysiwyg), we can forget with that.
Anyway, I guess that doesn't belong in this issue. Though after seeing this for the first time now, I don't know whether I'm really interested in this patch... ;)
Comment #71
stella CreditAttribution: stella commentedmarking needs review for the testbot
Comment #72
sunThe same in green.
Comment #73
sunOne more line less.
Comment #74
sign CreditAttribution: sign commentedRerolled #73, attribute class should be an array now, tested and works fine
Comment #75
Bojhan CreditAttribution: Bojhan commentedI am going to mark this RTBC now. Its passed many reviews, and now it is trying to keep up the ever changing core. We will need to user test this anyway.
Comment #76
webchickCould someone post some recent screenshots (or point me to where they are in the issue)? I have no idea what this patch does from its description, and I'm too sleepy to read the entire issue tonight. :)
Comment #77
sunWell. I'm not really sure what exactly this patch tries to implement (although I revamped it), but from a technical standpoint, it looks and feels very similar to the teaser splitter we had in D6. For text fields having an optional summary, you get a "Edit"/"Hide" summary link, which dynamically exposes a separate field for the field's summary.
Contrary to the teaser splitter in D6, this summary field already exists in the HTML markup (which is an improvement). However, it uses the same text format as the full text field, which means that one text format is guilty for two, separate textareas.
Aside from that off-topic rant, the introduced JavaScript allows to quickly edit/hide/set the optional, separate (forced) summary for a field. I can only guess that not entering something into the separate summary textarea is supposed to mean that Drupal takes over the automated generation of a "teaser" (summary).
Older screenshots are available in http://drupal.org/node/513414#comment-1859020
Comment #78
eigentor CreditAttribution: eigentor commentedRight. If you enter nothing, Drupal takes over summary creation. Sign, could you post some screenshots?
Comment #79
sign CreditAttribution: sign commentedRerolled the patch to work with latest head (esp. #557056: Removing the <body> fieldset around summary and body )
Please review spec. these three lines, if its the best way how to handle it
attached screenshots
Comment #80
sunLooks good. Thanks for re-rolling!
Comment #81
yoroy CreditAttribution: yoroy commentedI support this implementation. We'll need to user test it again, but keeping the summary field hidden initially, show only on request will definately be an improvement over the current situation.
Comment #83
catchComment #84
yched CreditAttribution: yched commentedwas RTBC ;-)
Comment #85
webchickIs it just me or does this patch not actually do anything? Tried on a clean install as well.
Comment #86
sign CreditAttribution: sign commented@webchick - you need to patch your drupal before installation, it adds a menu item to "enable edit mode"
Comment #87
webchicksign: Sorry? Was that a reply to the correct issue? There is no "enable edit mode" menu item with this patch applied prior to a clean install. Or do you mean I need #473268: D7UX: Put edit links on everything to see the effects of this patch..?
Comment #88
Bojhan CreditAttribution: Bojhan commentedTested, I can confirm it no longer applies.
Comment #89
webchickThat sounds like the correct status then.
The patch applies fine for me in terms of failed hunks, but doesn't actually /do/ anything. I wonder if something in the render system changed recently that doesn't allow the JS to attach itself to the page...
Comment #90
sign CreditAttribution: sign commented@webchick oh sorry, thought we are in different issue, apologies
investigating...
Comment #91
sign CreditAttribution: sign commented@webchick you were right, #attached_js changed
its now attached['js'][]
'#attached' => array('js' => array(drupal_get_path('module', 'text') . '/text.js')),
Comment #92
Bojhan CreditAttribution: Bojhan commentedConfirmed it works again. RTBC again, since there where no technical complaints.
Comment #93
webchickOk cool. I went ahead and committed this since it is light years better than what we have now.
It sounds like sun has some concerns that need to be addressed in a follow-up issue.
Additionally, I noticed while testing this patch that
<!--break-->
no longer does anything, and this is a regression from D6 and below. Currently in D6 and below, I can make summary/full view completely separate, I can let Drupal auto-generate it for me based on length, or I can selectively place the summary splitter within the full view and show the summary in both places. That last option appears to be missing now, and this will break the display of legacy sites. Unless I'm missing a big clue stick.Comment #94
heather CreditAttribution: heather commentedDoes this still "need usability testing"?
It was added to a list of *hot patches* for testing, but now it has been committed.
http://groups.drupal.org/node/26409
Do you need specific data on this?
Comment #95
catchYeah this is something which will need testing before release so we can refine it - it's more or less replacing the teaser splitter, which was confusing as hell, so important we don't repeat that for another release.