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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eigentor’s picture

Category: bug » task
eigentor’s picture

eigentor’s picture

Added jet another tag for the issues that need testing.... Anyone slap me if they think tagitis is making things worse.

eigentor’s picture

Issue tags: +Usability
eigentor’s picture

eigentor’s picture

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

tstoeckler’s picture

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

kika’s picture

Agree on summary/teaser field to be a collapsed field.

eigentor’s picture

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

yched’s picture

That would be a change for text.module, text_textarea_with_summary_process()

eigentor’s picture

OK. 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 :)

kika’s picture

Subscribing

kika’s picture

FileSize
1.83 KB

No, 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?

kika’s picture

Status: Active » Needs work
FileSize
1.23 KB

Whoopsie, now with less cruft

eigentor’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Bojhan’s picture

Priority: Normal » Critical

This is critical for Drupal 7.

catch’s picture

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

stBorchert’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
37 KB
33.68 KB

eigentor 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]".

tstoeckler’s picture

Even 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!).

catch’s picture

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

stBorchert’s picture

Yes, its not trimmed actually.
Maybe we could load the trimmed version with AJAX? And apply the current selected filter if we are on it?

eigentor’s picture

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

Bojhan’s picture

FileSize
17.21 KB
17.04 KB

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

yched’s picture

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

catch’s picture

I like both of yched's suggestions, don't necessarily have to be mutually exclusive.

stBorchert’s picture

Ok, here is a new one. I've tried to implement Bojhan's proposal.

Bojhan’s picture

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

stBorchert’s picture

Next try.
1. Done.
2. Yes. We need to add some AJAX magic to get the trimmed version of the body.
3. Done.
4. Uhm, (AFAIK) no.

yched’s picture

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

eigentor’s picture

FileSize
64.73 KB
96.44 KB

Aha, 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 :)

tic2000’s picture

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

stBorchert’s picture

Next patch without filling the summary automatically.

stella’s picture

Visual review of patch:

  • Pretty good overall in terms of coding style, etc.
  • Line 41 of resulting patched file (if ($('#edit-body-0-summary').val() == '') ) - you should use === here (see "Operators" section on JavaScript Coding Standards).
  • I'm concerned about the use of 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 have 0 in the class / id names?

I've tested it out and it works great, and degrades gracefully too :)

stBorchert’s picture

@stella
* Line 41 ... ok, shouldn't be that hard to fix
* body-0 ... yeah, I know. It is indeed body-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.

stella’s picture

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

stBorchert’s picture

Uhm, 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?

tic2000’s picture

@stella
In theory you can have more text with summary fields, but they won't be named body.

catch’s picture

"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

yched’s picture

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

Bojhan’s picture

So we have largely agreed on the behavior, I am thinking some text changes - but the rest is fine.

stella’s picture

Assigned: Unassigned » stella
Status: Needs review » Needs work

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

stella’s picture

FileSize
5.26 KB

Ok here's a new patch. To summarise what it does:

  • Adds a new file modules/field/modules/text/text.js (instead of patching node/node.js)
  • Modifies modules/field/modules/text/text.module so as to:
    • Load the new javascript file.
    • Add a class and a wrapper div to the summary and full text textareas. These were needed as the javascript was getting too complex, as a result of having no common, but sufficiently unique classes to identify elements by. This made traversing the html elements complicated and unreliable.
  • It can now handle textareas with summaries other than the body textarea.
  • It can also handle multiple textareas with summaries on the same page and with varying states.
stella’s picture

Status: Needs work » Needs review
yched’s picture

The approach is looking great, thanks stella.

+++ modules/field/modules/text/text.js	15 Aug 2009 00:33:14 -0000
@@ -0,0 +1,62 @@
+Drupal.behaviors.bodySummary = {

Having "Body" in the name is wrong ;-)

+++ modules/field/modules/text/text.js	15 Aug 2009 00:33:14 -0000
@@ -0,0 +1,62 @@
+          $(this).closest('fieldset').find('div.full-text-wrapper div.form-item-textarea label').append($('<span class="field-edit-link">(<a class="link-edit-summary" href="#">' + Drupal.t('Edit summary') + '</a>)</span>'));
+          $(this).closest('fieldset').find('a.link-edit-summary').click(function () {
+            $(this).closest('fieldset').find('div.summary-wrapper').show();
+            $(this).closest('fieldset').find('label span.field-edit-link').hide();
...

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.

stella’s picture

FileSize
4.93 KB

Ok here's a re-rolled version which addresses those 2 items.

yched’s picture

I forgot: instead of

drupal_add_js(drupal_get_path('module', 'text') . '/text.js');

it's preferable to use

'#attached_js' => array(drupal_get_path('module', 'text') . '/text.js')

as an entry in the $element array (better cacheability)

Bojhan’s picture

FileSize
4.94 KB

You mean this?

Status: Needs review » Needs work

The last submitted patch failed testing.

Bojhan’s picture

Status: Needs work » Needs review
FileSize
4.95 KB

Ok, fixing tests - and array position.

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
4.62 KB

re-rolled patch using #attached_js

catch’s picture

Title: User-Test Position of Teaser field above main text field in node form » Collapse summary field by default
stBorchert’s picture

FileSize
31.17 KB

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

Bojhan’s picture

Title: Collapse summary field by default » Collapsible summary field

Fixing the title. Would love if a field expert could give this a last review.

stella’s picture

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

stBorchert’s picture

Before 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 is 0 if its hidden just like $(this)[0].offsetWidth.

stella’s picture

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

sun’s picture

Title: Collapsible summary field » User-Test Position of Teaser field above main text field in node form
Status: Needs review » Needs work
+++ modules/field/modules/text/text.js	15 Aug 2009 11:18:15 -0000
@@ -0,0 +1,65 @@
+Drupal.behaviors.textareaSummary = {
...
+    $('textarea.summary-textarea').each(function () { 

Why 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().

+++ modules/field/modules/text/text.js	15 Aug 2009 11:18:15 -0000
@@ -0,0 +1,65 @@
+      var fieldset = $(this).closest('fieldset');
+      var div = $(this).closest('div.summary-wrapper');

Variables that are jQuery objects should be prefixed with $.

+++ modules/field/modules/text/text.js	15 Aug 2009 11:18:15 -0000
@@ -0,0 +1,65 @@
+      if ($(this).val() === '') {

Why type-agnostic?

+++ modules/field/modules/text/text.js	15 Aug 2009 11:18:15 -0000
@@ -0,0 +1,65 @@
+        $(div).hide();

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.

+++ modules/field/modules/text/text.js	15 Aug 2009 11:18:15 -0000
@@ -0,0 +1,65 @@
+        if ($(fieldset).find('div.full-text-wrapper').prev('label > span.field-edit-link').size()) {

Always use .length instead of .size().

+++ modules/field/modules/text/text.js	15 Aug 2009 11:18:15 -0000
@@ -0,0 +1,65 @@
+          $(fieldset).find('div.full-text-wrapper div.form-item-textarea label').append($('<span class="field-edit-link">(<a class="link-edit-summary" href="#">' + Drupal.t('Edit summary') + '</a>)</span>'));
+          $(fieldset).find('a.link-edit-summary').click(function () {
+            $(fieldset).find('div.summary-wrapper').show();
+            $(fieldset).find('label span.field-edit-link').hide();

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

stella’s picture

Status: Needs work » Needs review
FileSize
4.99 KB

Here'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 a textarea-summary-processed class instead to prevent it being processed more than once.

sun’s picture

+++ modules/field/modules/text/text.js	16 Aug 2009 20:24:03 -0000
@@ -0,0 +1,72 @@
+  attach: function () {
+    $('textarea.textarea-summary:not(.textarea-summary-processed)').each(function () {

The context argument is missing here. The attach function should also have the new 'settings' argument for consistency.

+++ modules/field/modules/text/text.js	16 Aug 2009 20:24:03 -0000
@@ -0,0 +1,72 @@
+    $('textarea.textarea-summary:not(.textarea-summary-processed)').each(function () {
...
+      // Mark this textarea as processed.
+      $(this).addClass('textarea-summary-processed');

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

+++ modules/field/modules/text/text.module	16 Aug 2009 20:24:04 -0000
@@ -664,6 +664,10 @@ function text_textarea_with_summary_proc
+    '#attached_js' => array(drupal_get_path('module', 'text') . '/text.js'), 
+    '#attributes' => array('class' => 'textarea-summary'),
+    '#prefix' => '<div class="summary-wrapper">',
@@ -676,6 +680,9 @@ function text_textarea_with_summary_proc
+    '#attributes' => array('class' => 'full-text-textarea'),
+    '#prefix' => '<div class="full-text-wrapper">',

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.

stella’s picture

FileSize
5 KB

Patch re-roll.

sun’s picture

+++ modules/field/modules/text/text.js	16 Aug 2009 22:14:26 -0000
@@ -0,0 +1,70 @@
+ * Automatically hide the summary textarea if it is empty.  Display links to
+ * hide and unhide the summary textarea.

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

+++ modules/field/modules/text/text.js	16 Aug 2009 22:14:26 -0000
@@ -0,0 +1,70 @@
+  attach: function (context, settings) {
+    $('textarea.text-textarea-summary:not(.text-textarea-summary-processed)').addClass('text-textarea-summary-processed').each(function () {

Thanks for adding context!

Now we just have to use it as second argument to $() :)

14 days to code freeze. Better review yourself.

stella’s picture

FileSize
4.96 KB

Damn, sorry for missing that. Patch re-roll.

Bojhan’s picture

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

sun’s picture

FileSize
5.24 KB

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

yched’s picture

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

Bojhan’s picture

Status: Needs work » Reviewed & tested by the community

Yup, working - just tested. - Oops didnt see yched comment.

sun’s picture

Status: Needs review » Needs work
sun’s picture

First pass.

Though on a side note: Oh dear. I'm entirely not happy with this widget and the underlying implementation.

function text_textarea_with_summary_process($element, $form_state, $form) {
...
  if (!empty($instance['settings']['text_processing'])) {
    $filter_key  = (count($element['#columns']) == 2) ? $element['#columns'][1] : 'format';
    $format = isset($element['#value'][$filter_key]) ? $element['#value'][$filter_key] : FILTER_FORMAT_DEFAULT;
    $element[$field_key]['#text_format'] = $format;
  }

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

stella’s picture

Status: Reviewed & tested by the community » Needs review

marking needs review for the testbot

sun’s picture

The same in green.

sun’s picture

One more line less.

sign’s picture

Rerolled #73, attribute class should be an array now, tested and works fine

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Could 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. :)

sun’s picture

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

eigentor’s picture

Right. If you enter nothing, Drupal takes over summary creation. Sign, could you post some screenshots?

sign’s picture

Rerolled 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

var $fieldset = $(this).closest('#body-wrapper');
var $summaryLabel = $summary.find('div.form-type-textarea label');
var $fullLabel = $full.find('div.form-type-textarea label');

attached screenshots

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Thanks for re-rolling!

yoroy’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Reviewed & tested by the community

was RTBC ;-)

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Is it just me or does this patch not actually do anything? Tried on a clean install as well.

sign’s picture

@webchick - you need to patch your drupal before installation, it adds a menu item to "enable edit mode"

webchick’s picture

sign: 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..?

Bojhan’s picture

Tested, I can confirm it no longer applies.

webchick’s picture

Status: Needs review » Needs work

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

sign’s picture

@webchick oh sorry, thought we are in different issue, apologies

investigating...

sign’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

@webchick you were right, #attached_js changed
its now attached['js'][]

'#attached' => array('js' => array(drupal_get_path('module', 'text') . '/text.js')),

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed it works again. RTBC again, since there where no technical complaints.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

heather’s picture

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

catch’s picture

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

Status: Fixed » Closed (fixed)
Issue tags: -Usability, -Needs usability testing

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