#23261: implement configuration portion of new Tor Launcher UI --------------------------------------------+------------------------------ Reporter: mcs | Owner: brade Type: defect | Status: | needs_revision Priority: Medium | Milestone: Component: Applications/Tor Launcher | Version: Severity: Normal | Resolution: Keywords: ux-team, TorBrowserTeam201710R | Actual Points: Parent ID: #21951 | Points: Reviewer: | Sponsor: Sponsor4 --------------------------------------------+------------------------------ Changes (by gk):
* status: needs_review => needs_revision Comment: Replying to [comment:30 mcs]: > Replying to [comment:29 gk]: > > First part of the review. This only covers comparing the attached design spec with the layout implemented. Apart from nits which probably don't matter (slightly different "Quit" button positioning etc.): > > The position of the Quit button varies by platform due to our use of Mozilla's XUL wizard element. We could change it to be consistent across platforms, but it would be extra work and might make things less consistent with other parts of Tor Browser. Yeah, don't do that. > > 1) The design shows that `obfs4` is selected by default and that it is the recommended bridge type. Both is missing in the layout implemented but it seems worthwhile to have, no? > > We implemented v10 of Linda's design. I think you may be looking at v9. I will attach the v10 PDF to this ticket (it was sent by Linda to some of us via email on Sept 26th). Heh. It seems I have been one of them. Thanks for adding v10 to this ticket, so others have the latest design draft available as well. > > 2) Is there are ticket somewhere tracking the fancy icons and progress implementation as shown on page 9 and 10 of the design doc? If not, could you create one? > > I created #23971. Thanks. Okay. I went over all three commits and they look good to me apart from minor issues. Great work! I'll post all my comments here to not have them scattered over different tickets. c6fd54b8ba36bf88630db446c68ba8d04ed9727a 1)`<vbox class="firstResponses" align="center" >` <- whitespace before ">" 2) I was a bit confused by the mix of of " />" and "/>", the former seems to be for older code? Just as a note that's not something that needs to get fixed 3) please fix `TODO2017` items or file new tickets 1ebd091f1e185ccf3ffaa739bf8ec232447ead8a 1) Here you are not doing the "var" -> "let" renaming as in the previous commit, any reason for that? (not necessarily something to fix) 2) no mixed {} if-else clauses (see `readTorSettings()`) ba78bfa36ebac398511ddde1ece70f82d202383a 1) no mixed {} if-else clauses (see `_configureDefaultBridges` in tl- process.js and `showAlert()` in tl-util.jsm) One bug I found (it's not new though): 1) Start with connecting directly. 2) Open the Tor Launcher network settings in the browser window and check the firewall option 3) Click okay and restart the browser 4) Cancel normal start-up and configure bridges (obfs4) 5) Check the Tor Launcher network settings in the browser window and the firewall option is now unchecked (which should not be the case) I am still chasing two other bugs which I failed to trigger again, so more might come. :) -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23261#comment:33> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs