There are at least two bugs here:

1. It mentions PHPBB. It should not mention another forum package. ;P
2. It mentions taxonomy/comment being required, but that's handled by dependencies now.

Comments

neclimdul’s picture

Status: Active » Needs review
StatusFileSize
new2.78 KB

I'll take a shot at it.

The listed complaints are taken care of and I tried making the content more readable and better organized as well.

dries’s picture

Status: Needs review » Needs work

"Forum topics can also have their own URLs like other content types."

Small remark: a forum topic is not a 'content type'. I think you meant 'content'.

Otherwise looks OK.

webchick’s picture

"content type" would be consistent with the menu item administer >> content >> content types, but why don't we just remove that sentence altogether because it doesn't really add anything. However, you always want 3 items in a list, so I'm not sure what that other item should be.

How about something like "You may lock a thread by turning the commenting option to disabled on the parent thread." I've seen that come up on the forums every now and then, and it more closely matches the rest of the items there, as it's talking about a specific administrative action you can do.

neclimdul’s picture

Status: Needs work » Needs review
StatusFileSize
new3.62 KB

I like webchick's third item and have modified the patch to include that instead of the URL bit. That's really path help anyways and might confuse people if the path module isn't turned on.

drumm’s picture

Status: Needs review » Needs work

patching file modules/forum/forum.module
Hunk #1 FAILED at 1.
Hunk #3 FAILED at 28.
2 out of 3 hunks FAILED -- saving rejects to file modules/forum/forum.module.rej

webchick’s picture

Version: 5.x-dev » 6.x-dev

We're in a string freeze ... this can't get applied until 6.x

webchick’s picture

Component: forum.module » documentation

Moving to documentation component.

sutharsan’s picture

Status: Needs work » Needs review
StatusFileSize
new2.76 KB

Patch re-rolled against current 6.x-dev.

keith.smith’s picture

Status: Needs review » Needs work

This mostly looks ok, except the first paragraph is a little clunky, though less so after this patch. I don't have any concrete suggestions about it right this moment though.

In the second paragraph, the text:

Forums can be organized under what are called <em>containers</em>.

could probably just be

Forums can be organized under <em>containers</em>.
keith.smith’s picture

webchick hit the major point here in when she created the issue:

There are at least two bugs here:

1. It mentions PHPBB. It should not mention another forum package. ;P
2. It mentions taxonomy/comment being required, but that's handled by dependencies now.

This patch does remove PHPBB, and takes out the taxonomy/comments stuff that is already handled if forum has been enabled. However, I also fiddled with most (or all) of the texts. Forum is a strange one, because it has several tough concepts that all blend together. Containers containing forums with forum topics that have comments (and replies).

Patch attached for review.

keith.smith’s picture

Status: Needs work » Needs review
keith.smith’s picture

Status: Needs review » Needs work

OK. So not so much. I'll attach a patch when the issue directory comes back online.

keith.smith’s picture

Status: Needs work » Needs review
StatusFileSize
new13.72 KB

And, here we go. This patch goes with the comments in #10.

catch’s picture

Looks pretty good to me. Only thing I'd say is I didn't notice a mention of drag and drop?

keith.smith’s picture

Status: Needs review » Needs work

sigh. yeah, I forgot about that.

keith.smith’s picture

Status: Needs work » Needs review
StatusFileSize
new13.95 KB

Ok. To the admin/content/forum page, this adds:

To rearrange forums and containers, grab a drag-and-drop handle under the Name column and drag the forum or container to its new location. (Grab a handle by clicking and holding the mouse while hovering over a handle icon.)

Is this sufficient for drag-and-drop? If so, I'll use this as a model on....uh...the block, menu, taxonomy and profile pages.

Note:

  • This does not handle non-js enabled browsers (but we don't present different help text for them). I guess we could say In most browsers, to rearrange forums and containers....
  • Wikipedia says "drag-and-drop" with hyphens, and an internet full of folks can't be wrong.
  • We can't say "left side of the list" because the table may be RTL (and presumably the handle would be on the right). I guess we could say "edge" and leave it at that.
  • I purposely left off which mouse button to click -- lots of left-handed folks reverse their mouse buttons so left is not always accurate. Back in the day, Macs only had one button (though I guess they have two now AFAIK) :)
  • There's no mention of drag-and-drop on the actual help page, though I'm not sure that is necessary.
  • Edit: And oh...we can't really say "the handle" and the like, because there may be lots of them, or none.
michelle’s picture

Since I don't have Drupal 6, I only read through the patch file. This also means I can't comment on the accuracy of text related to drag and drop or other content types.

In general, I thought the text was much improved. I only had two small nitpicks:

"Description of or guidelines for" - This may just be me but that wording is jarring.

"selecting Disabled under Comment settings while editing a forum topic will unpublish all existing comments on the thread, and prevent new ones." - This isn't true in 5.x. I don't know if it is in 6.x.

If someone with 6.x could check whether comments are unpublished to verify that part, I think this is RTBC.

Michelle

keith.smith’s picture

Status: Needs review » Needs work

Yes, I used the wrong term there.

keith.smith’s picture

StatusFileSize
new13.82 KB

Ok, addressed both of these issues:
* Disabled hides comments, rather than unpublishes them
* Changed the Description of or guidelines for to Description and guidelines for

keith.smith’s picture

Status: Needs work » Needs review

meh.

keith.smith’s picture

StatusFileSize
new13.95 KB

Added Remember that your changes will not be saved until you click the Submit button at the bottom of the page. on the drag-and-drop description.

Note that Submit should perhaps be Save and that Reset to Alphabetical should be Reset to alphabetical (and should work) but that's another issue.

gábor hojtsy’s picture

Status: Needs review » Needs work

- IMHO there is information loss with replacing "delete all sub-forums and associated posts as well" with "also delete all of its contents". Even if the former was not exactly nice, it gives us more info (posts *will* be deleted). This is not clear in the new version. There are multiple instances of this in the patch.

- "Minimum number of comments necessary to be considered "hot"." What is considered? I am not sure the field title gives enough context.

- We avoid glueing stuff together like "@createtopic", instead we do "@create-topic" or something along those lines, maybe even @create.

- "contained within a container" :)

- the list items have ; at the end while other similar lists eg. in block module have dots if I remember right(?)

- Let's name the button "Save forums" or "Save" (I know there is an ongoing debate about this) with this patch, so the help text will also be right at the same time!

- I like the 'drag handler under named column' type of way of talking about the drag and drop which is compatible with RTL, although if an RTL theme is used, hopefully it will be accompanied with an appropriate translation, so it will be translated to however it is positioned. :) Anyway, this looks like a good standard even thinking about possible form_alter()s, which might modify how the form looks.

- @forum_vocab same as with @createtopic, use @vocabulary here or something like this.

Generally, these are not huge showstoppers, but it would be great, to get these right, and finally remove the phpBB reference, so keep up the great work :)

keith.smith’s picture

Great, thanks for the review. I'll see about getting this fixed up ASAP.

I had thought -- much later, after I had already posted -- that obviously, RTL translators would likely change left to right in their translations. So I'm not certain that's as big a deal as I thought.

Slightly off topic, as an update:

I know the string freeze is imminent: as of the weekend, we were lacking major help text rewrites for: node, block, system, menu and locale. Drag and drop needed to be added to forum, and profile. There are also some less pressing issue for contact, comment, filter and (one part of) taxonomy.

Locale and block are in, and most of the rest of the major ones need a review, or are, like this one, recovering from a review. :)

The only major one that I am aware of that where there's not a good patch at or near some stage of completion is system, and man, does that text needs help. I'm going to get something in the queue for it today though, unless someone else beats me to it.)

keith.smith’s picture

Status: Needs work » Needs review
StatusFileSize
new14.09 KB

I have the feeling I forgot something here, as I am trying to hurriedly put this back into review (am late for dinner).

- the delete confirmation messages were less informative, so I put them back to near what they were.
- same for the hot description "hot" (considered was in there originally)
- fixed "@createtopic" and the legacy instance of "@forum_vocab"
- This should no longer contain "contained within a container". :)
- added periods to the ends of list items.
- named the button "Save" rather than "Save forums" since it also appears on taxonomy forms. Fixed the reference to it in the help. I did a bit of basic testing and the rename does not appear to have broken anything.

I think that was everything...

gábor hojtsy’s picture

Status: Needs review » Fixed

I looked through this one, and all my comments were acted on, so it looked fine. Even the explanation of what is removed when a forum gets deleted was expanded on, even though you did not mention it in your list. Thanks, committed.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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