Closed Bug 731223 Opened 8 years ago Closed 8 years ago

Update help for changes from bug #694514 and bug#733258.

Categories

(SeaMonkey :: Help Documentation, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.10

People

(Reporter: ewong, Assigned: ewong)

References

Details

Attachments

(2 files, 3 obsolete files)

Bug #694514 will have exposed mailnews.reply_header.  Update help to reflect these changes.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #602822 - Flags: review?(jh)
Comment on attachment 602822 [details] [diff] [review]
Update help for changes from bug 694514. (v1)

Review of attachment 602822 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nit addressed *and* matching the UI (I strongly feel bug 694514 needs some tweaks, see Serge's and my comments there - better fix the UI first and then Help).

::: suite/locales/en-US/chrome/common/help/mailnews_preferences.xhtml
@@ +291,5 @@
> +      <ol><li>No Reply Header</li>
> +          <li>[Author] wrote:</li>
> +          <li>On [date] [author] wrote:</li>
> +          <li>[Author] on [date] wrote:</li>
> +      </ol></li>

Nit: The ordered list start and end tags should be on their own lines, with no additional indentation for themselves. So:
First level <li>, second level text and the ol start/end tags, third level the subordinate li items. The closing tag for the outer li item shall also be on its own line in this case (cf. line 90 in the same file).
Attachment #602822 - Flags: review?(jh) → review+
Version: SeaMonkey 2.8 Branch → Trunk
Depends on: 733258
Comment on attachment 602822 [details] [diff] [review]
Update help for changes from bug 694514. (v1)

Sorry, I changed my mind. Actually I'd like to see a new patch (once the exact wording of the options has been clarified) which shall also explain the relationship to the mailnews.reply_header_* prefs. Otherwise it won't be clear why there will be a capital "On" after the comma with option 4 (by default).

I'll put references to where the prefs can be found on bug 733258.
Attachment #602822 - Flags: review+ → review-
Attachment #602822 - Attachment is obsolete: true
Attachment #604349 - Flags: review?(jh)
Comment on attachment 604349 [details] [diff] [review]
Update help for changes from bug 694514. (v2)

Review of attachment 604349 [details] [diff] [review]:
-----------------------------------------------------------------

r- since I want to see a new patch. Bear with me, we're almost there. :-)

::: suite/locales/en-US/chrome/common/help/mailnews_preferences.xhtml
@@ +285,5 @@
>      send the message when you&apos;re pressing Ctrl+Return in message editor.
>      This may help you avoid accidentally sending the message if you enter the
>      keyboard shortcut by mistake when composing a message.</li>
> +  <li><strong>Select reply header type</strong>:
> +    Select the type of reply header you wish to use.  There are four choices

Nit: Double space before "There".

@@ +292,5 @@
> +      <li>No Reply Header</li>
> +      <li>[Author] wrote:
> +        <ul>
> +          <li>This setting is based on the mailnews.reply_header_authorwrote preference.
> +          </li>

I don't see the benefit of an extra ul/li for the explanations (this one and the other two); a list only makes sense if it has at least two items. I think it would be better/best to just use a <br> after the option string instead.

@@ +299,5 @@
> +      <li>On [date], [author] wrote:
> +        <ul>
> +          <li>This setting is based on the mailnews.reply_header_ondate preference combined with
> +              the mailnews.reply_header_authorwrote preference separated by the mailnews.reply_header_separator
> +              preference option.

Nit (applied to this item and the next): I think "option" is redundant there, and "preference" used a little too often. Suggestion: "This setting is based on a combination of the X, Y and Z preferences." [I think the name of the third pref speaks for itself.]
Attachment #604349 - Flags: review?(jh) → review-
Summary: Update help for changes from bug #694514. → Update help for changes from bug #694514 and #733258.
Summary: Update help for changes from bug #694514 and #733258. → Update help for changes from bug #694514 and bug#733258.
Attachment #604349 - Attachment is obsolete: true
Attachment #604820 - Flags: review?(jh)
Comment on attachment 604820 [details] [diff] [review]
Update help for changes from bug 694514 and bug 733258 (v3)

Review of attachment 604820 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits fixed. Thanks!
Note: If you for some reason cannot check in this and/or bug 733258 *today*, please set checkin-needed accordingly since it needs to land ASAP (Aurora uplift = string freeze is *tomorrow*).

::: suite/locales/en-US/chrome/common/help/mailnews_preferences.xhtml
@@ +289,5 @@
> +    Select the type of reply header you wish to use. There are four choices
> +    available:
> +    <ol>
> +      <li>No Reply Header
> +      </li>

(Here and the other three times below:) The closing list item tag needs to be at the end of the line directly above. Basically the rule is: At the end of the line if the line contains text, with no space between the text and the closing tag.

@@ +292,5 @@
> +      <li>No Reply Header
> +      </li>
> +      <li>[Author] wrote:<br/>
> +         This setting is based on the mailnews.reply_header_authorwrote preference.
> +      </li>

Same as above, but if the line gets longer than 80 characters, please wrap accordingly.

@@ +296,5 @@
> +      </li>
> +      <li>On [date], [author] wrote:<br/>
> +         This setting is based on a combination of the mailnews.reply_header_ondate, 
> +         mailnews.reply_header_separator and mailnews.reply_header_authorwrote preferences.
> +      </li>

Same as above, but needs wrapping where possible (I think in this case before "preferences").

@@ +300,5 @@
> +      </li>
> +      <li>[Author] wrote, On [date]:<br/>
> +         This setting is based on a combination of the mailnews.reply_header_authorwrote,
> +         mailnews.reply_header_separator and mailnews.reply_header_ondate preferences.
> +      </li>

Same as above.
Attachment #604820 - Flags: review?(jh) → review+
Fixed nits from comment #7.
Attachment #604820 - Attachment is obsolete: true
Attachment #604845 - Flags: review+
Comment on attachment 604845 [details] [diff] [review]
Update help for changes from bug 694514 and bug 733258 (v4)

Review of attachment 604845 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/locales/en-US/chrome/common/help/mailnews_preferences.xhtml
@@ +290,5 @@
> +    available:
> +    <ol>
> +      <li>No Reply Header</li>
> +      <li>[Author] wrote:<br/>
> +         This setting is based on the mailnews.reply_header_authorwrote 

(Bah, I'm very sorry, saw this before but forgot; here and below:) This should be one space less indent (2 spaces for each level).
Fixed nits from previous patch.
Attachment #604850 - Flags: review?(jh)
Comment on attachment 604850 [details] [diff] [review]
Update help for changes from bug 694514 and bug 733258 (part 2) (v1)

(Please land as a single patch.)
Attachment #604850 - Flags: review?(jh) → review+
Pushed to comm-central:
(first r+ patch)
  http://hg.mozilla.org/comm-central/rev/2af54b6c02f2

(part 2)
  http://hg.mozilla.org/comm-central/rev/af896f514c15
  (thanks to a .hgrc screwup, it's attributed to builder. )
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Backed out from comm-central: (due to author misattribution)
 http://hg.mozilla.org/comm-central/rev/988c0c1fd2f4

Pushed to comm-central:
 http://hg.mozilla.org/comm-central/rev/e74ad835b0f4
Target Milestone: --- → seamonkey2.10
You need to log in before you can comment on or make changes to this bug.