Closed Bug 1565789 Opened 5 years ago Closed 5 years ago

Remove the aTabID in openPreferencesTab()

Categories

(Thunderbird :: Preferences, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 2 obsolete files)

With bug 1498332 we remove the tabs in the pref panes. This makes the aTabID no more needed and we should remove it.

Attached patch 1565789-update-prefs-callers.patch (obsolete) (deleted) — Splinter Review

I had a better idea. Instead of removing the tabID, use it as an ID which scrolls into the view of the prefs (focusID). Like this the user doesn't need to find the correct section to work on it. Especially with long panes this is a big help.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9077934 - Flags: review?(geoff)
Attached patch 1565789-update-prefs-callers.patch (obsolete) (deleted) — Splinter Review

Forgot a period.

Attachment #9077934 - Attachment is obsolete: true
Attachment #9077934 - Flags: review?(geoff)
Attachment #9077987 - Flags: review?(geoff)
Comment on attachment 9077987 [details] [diff] [review] 1565789-update-prefs-callers.patch Review of attachment 9077987 [details] [diff] [review]: ----------------------------------------------------------------- This is good but consider using "scroll" ("scrollTarget"?) where you've used "focus" since we're not focussing anything. ::: mail/base/content/mailWindowOverlay.js @@ +2910,5 @@ > * The dialog window will take care of reloading the message when > * invoked in instantApply mode. > */ > function OpenPhishingSettings() { > + openOptionsDialog("panePrivacy"); Second argument missing? ::: mail/components/compose/content/MsgComposeCommands.js @@ +2222,5 @@ > // Construct the notification as we don't have one. > let msg = document.createXULElement("hbox"); > msg.setAttribute("flex", "100"); > msg.onclick = function(event) { > + openOptionsDialog("paneCompose", Second argument missing. ::: mail/components/preferences/preferences.js @@ +108,3 @@ > * @param otherArgs.subdialog ID of button to activate, opening a subdialog > */ > +function selectPaneAndTab(prefWindow, paneID, focusID, otherArgs) { This should be renamed. Maybe just selectPane. @@ +127,3 @@ > * @param subdialogID ID of button to activate, opening a subdialog > */ > +function showTab(pane, focusID, subdialogID) { This should be renamed too (scrollPaneTo?), and the first argument is now unused.
Attachment #9077987 - Flags: review?(geoff) → review+

I'm using now scrollPaneTo instead of focusID.
(In reply to Geoff Lankow (:darktrojan) from comment #3)

Comment on attachment 9077987 [details] [diff] [review]
1565789-update-prefs-callers.patch

Review of attachment 9077987 [details] [diff] [review]:

This is good but consider using "scroll" ("scrollTarget"?) where you've used
"focus" since we're not focussing anything.

::: mail/base/content/mailWindowOverlay.js
@@ +2910,5 @@

  • The dialog window will take care of reloading the message when
  • invoked in instantApply mode.
    */
    function OpenPhishingSettings() {
  • openOptionsDialog("panePrivacy");

Second argument missing?

Fixed.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2222,5 @@

// Construct the notification as we don't have one.
let msg = document.createXULElement("hbox");
msg.setAttribute("flex", "100");
msg.onclick = function(event) {

  • openOptionsDialog("paneCompose",

Second argument missing.

Not really needed as the target is the subdialog. But it's better when we show the area with the button.

::: mail/components/preferences/preferences.js
@@ +108,3 @@

  • @param otherArgs.subdialog ID of button to activate, opening a subdialog
    */
    +function selectPaneAndTab(prefWindow, paneID, focusID, otherArgs) {

This should be renamed. Maybe just selectPane.

Renamed it to selectPrefPane to be unique. selectPane exists already at other places.

@@ +127,3 @@

  • @param subdialogID ID of button to activate, opening a subdialog
    */
    +function showTab(pane, focusID, subdialogID) {

This should be renamed too (scrollPaneTo?), and the first argument is now
unused.

Done.

Attachment #9077987 - Attachment is obsolete: true
Attachment #9078238 - Flags: review+

This needs to land together with bug 1498332.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a912193380b5
Update the preferences callers after the prefs redesign in bug 1498332. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Regressions: 1677647
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: