Closed Bug 1679268 Opened 4 years ago Closed 4 years ago

remove <deck> from subscribe.xhtml

Categories

(Thunderbird :: General, task)

Tracking

(thunderbird_esr78 wontfix, thunderbird84 wontfix)

RESOLVED FIXED
85 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird84 --- wontfix

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(2 files, 5 obsolete files)

Remove <deck> from subscribe.xhtml: https://searchfox.org/comm-central/rev/f8c11a36225bbe5e145f1a9a0a3801428e265715/mailnews/base/content/subscribe.xhtml#82

Two children of fixed size, can just instead show/hide the one we need as appropriate.

Attachment #9189721 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #9189721 - Flags: review?(mkmelin+mozilla) → review?(alessandro)
Comment on attachment 9189721 [details] [diff] [review] Bug-1679268_de-deck-subscribe-xhtml-0.patch Review of attachment 9189721 [details] [diff] [review]: ----------------------------------------------------------------- This is good, but I think we should improve a bit the code to be more modern and readable. Let's update the comments for the methods we're updating to improve inline documentation. Also, please update the commit message with "Bug 1679268 - Remove the XUL <deck> element from the subscribe.xhtml dialog. r=aleca" ::: mailnews/base/content/subscribe.js @@ +267,3 @@ > > function InSearchMode() { > // search is the second card in the deck nit: Update the comment to reflect the new layout since we don't use the deck anymore. Also, start with a capitalized letter and finish with a period. @@ +434,1 @@ > } I think we can simplify and collapse the SwitchToNormalView and SwitchToSearchView method into one called toggleSubscriptionView(toggle). The toggle attribute could be a boolean that we can use this way: document.getElementById("normalview").hidden = toggle; document.getElementById("searchview").hidden = !toggle; @@ +434,4 @@ > } > > function Search() { > var searchValue = gNameField.value; nit: let ::: mailnews/base/content/subscribe.xhtml @@ +79,4 @@ > orient="vertical"> > <label id="subscribeLabel"/> > <hbox flex="1"> > + <hbox id="normalview" flex="1"> let's call this ID subscribeView for consistency. @@ +102,5 @@ > + </treecols> > + <treechildren id="subscribeTreeBody"/> > + </tree> > + </hbox> > + <hbox id="searchview" flex="1" hidden="true"> Let's use camelCase for IDs: searchView
Attachment #9189721 - Flags: review?(alessandro) → feedback+
Attachment #9189721 - Attachment is obsolete: true
Attachment #9189925 - Flags: review?(alessandro)
Attachment #9189925 - Attachment is obsolete: true
Attachment #9189925 - Flags: review?(alessandro)
Attachment #9189939 - Flags: review?(alessandro)
Comment on attachment 9189939 [details] [diff] [review] Bug-1679268_de-deck-subscribe-xhtml-2.patch Review of attachment 9189939 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/content/subscribe.js @@ +420,4 @@ > gSearchTree.invalidate(); > } > > +function toggleSubscriptionView(toggle) { Please us JSDocs to document the method and explain the parameter used. @@ +441,4 @@ > gSearchTree.view = gSearchView; > } > } else { > + toggleSubscriptionView(false); Here you can use a return; inside the first condition and avoid the else callback.
Attachment #9189939 - Flags: review?(alessandro) → feedback+
Attachment #9189939 - Attachment is obsolete: true
Attachment #9190093 - Flags: review?(alessandro)
Comment on attachment 9190093 [details] [diff] [review] Bug-1679268_de-deck-subscribe-xhtml-3.patch Review of attachment 9190093 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
Attachment #9190093 - Flags: review?(alessandro) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/95215a32c31f
Remove the XUL <deck> element from the subscribe.xhtml dialog. r=aleca

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch

Looks like this has broken comm/mail/test/browser/subscribe/browser_newsFilter.js

./mach test comm/mail/test/browser/subscribe/browser_newsFilter.js

Marionette TRACE Received observer notification toplevel-window-ready
0:08.97 GECKO(52978) JavaScript error: resource://testing-common/mozmill/SubscribeWindowHelpers.jsm, line 71: TypeError: can't access property "getColumnFor", tree.columns is null
0:08.98 INFO Console message: [JavaScript Error: "TypeError: can't access property "getColumnFor", tree.columns is null" {file: "resource://testing-common/mozmill/SubscribeWindowHelpers.jsm" line: 71}]
check_newsgroup_displayed@resource://testing-common/mozmill/SubscribeWindowHelpers.jsm:71:17
filter_test_helper/<@chrome://mochitests/content/browser/comm/mail/test/browser/subscribe/browser_newsFilter.js:54:36
waitFor@resource://testing-common/mozmill/utils.jsm:155:45
filter_test_helper@chrome://mochitests/content/browser/comm/mail/test/browser/subscribe/browser_newsFilter.js:53:9
callback@resource://testing-common/mozmill/SubscribeWindowHelpers.jsm:41:14
startTest@resource://testing-common/mozmill/WindowHelpers.jsm:343:16
WindowWatcher_notify@resource://testing-common/mozmill/WindowHelpers.jsm:380:9
Subscribe@chrome://messenger/content/mailCommands.js:367:10
MsgSubscribe@chrome://messenger/content/mailWindowOverlay.js:2178:14
oncommand@chrome://messenger/content/messenger.xhtml:1:1
MozMillController.prototype.click@resource://testing-common/mozmill/controller.jsm:391:13
wrapperFunc@resource://testing-common/mozmill/WindowHelpers.jsm:1343:32
open_subscribe_window_from_context_menu@resource://testing-common/mozmill/SubscribeWindowHelpers.jsm:44:6
test_subscribe_newsgroup_filter@chrome://mochitests/content/browser/comm/mail/test/browser/subscribe/browser_newsFilter.js:36:42
Tester_execTest/<@chrome://mochikit/content/browser-test.js:1069:34
Tester_execTest@chrome://mochikit/content/browser-test.js:1109:11
nextTest/<@chrome://mochikit/content/browser-test.js:932:14
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1037:23

Flags: needinfo?(khushil324)
Flags: needinfo?(khushil324)
Attachment #9190543 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9190543 [details] [diff] [review] Bug-1679268_follow-up-test-fail-browser-newsFilter-0.patch Review of attachment 9190543 [details] [diff] [review]: ----------------------------------------------------------------- Arbitrary waits are usually a problem. I think we should just make the wait function a bit more robust. Patch coming up
Attachment #9190543 - Flags: review?(mkmelin+mozilla) → review-
Attachment #9190543 - Attachment is obsolete: true
Attachment #9190568 - Flags: review?(khushil324)

Linted.

Attachment #9190568 - Attachment is obsolete: true
Attachment #9190568 - Flags: review?(khushil324)
Attachment #9190569 - Flags: review?(khushil324)
Comment on attachment 9190569 [details] [diff] [review] bug1679268_newstubscribe_testfix.patch Review of attachment 9190569 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Test passing on the local machine. r=khushil
Attachment #9190569 - Flags: review?(khushil324) → review+
Pushed by mkmelin@iki.fi: https://hg.mozilla.org/comm-central/rev/9df3b3b34a08 followup - make the newsgroup shown test more robust. r=khushil
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: