Closed
Bug 429667
Opened 17 years ago
Closed 17 years ago
make EnableTextbox() available in all prefpanes
Categories
(SeaMonkey :: Preferences, defect)
SeaMonkey
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0a1
People
(Reporter: bugzilla, Assigned: bugzilla)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
EnableTextbox(aTextboxId, aValue, aFocus) can be used in all prefpanes, where a checkbox is used to enable/disable a textbox, so it's useful to have it available everywhere and don't have redundant code.
Assignee | ||
Comment 1•17 years ago
|
||
Adds a new preferences.js and loads its content into preferences.xul.
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Attachment #316423 -
Flags: superreview?(neil)
Attachment #316423 -
Flags: review?(iann_bugzilla)
Comment 2•17 years ago
|
||
Comment on attachment 316423 [details] [diff] [review]
patch v1
Some uncalled for nits: ;-)
>Index: suite/common/pref/preferences.js
>===================================================================
>+// The contents of this file will be loaded into the scope of the
>+// prefwindow and is available in all prefpanes!
"content ... be available to ..."
But IanN and Neil should know even better. ;-)
>+function EnableTextbox(aTextboxId, aValue, aFocus)
I'd rename aValue to aCheckboxValue to make its use more clear.
>+ let enable = aValue && !pref.locked;
I'd use "enabled", because it's a state.
>Index: suite/common/pref/preferences.xul
>===================================================================
> <stringbundle id="bundle_prefutilities"
> src="chrome://communicator/locale/pref/prefutilities.properties"/>
>
>+ <script type="application/x-javascript" src="chrome://communicator/content/pref/preferences.js"/>
>+
The <script> should be the first child.
Comment on attachment 316423 [details] [diff] [review]
patch v1
r=me with mnyromyr's nits fixed
Attachment #316423 -
Flags: review?(iann_bugzilla) → review+
Comment 4•17 years ago
|
||
Comment on attachment 316423 [details] [diff] [review]
patch v1
I don't quite agree with Mnyromyr!
>+// The contents of this file will be loaded into the scope of the
>+// prefwindow and is available in all prefpanes!
I'd swap things so you get "is loaded" and "will be available".
>+function EnableTextbox(aTextboxId, aValue, aFocus)
I'd call it aEnable rather than aCheckbox.
Attachment #316423 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 5•17 years ago
|
||
Patch, addressing the review comments (I took Neil's suggestions).
Attachment #316423 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•17 years ago
|
||
Thx for landing -> RESOLVED FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•