Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(159)

Issue 10907148: Implement popup bubbles for the controlled setting indicator (Closed)

Created:
8 years, 3 months ago by bartfab (slow)
Modified:
8 years, 2 months ago
Reviewers:
flackr, kevers, Evan Stade
CC:
chromium-reviews, arv (Not doing code reviews), stevenjb+watch_chromium.org, oshima+watch_chromium.org
Visibility:
Public.

Description

Implement popup bubbles for the controlled setting indicator This CL adds support for popup bubbles to the settings UI and makes the options.ControlledSettingIndicator show such a bubble when it is clicked. The options.Bubble class inherits from cr.ui.BubbleBase, allowing all style and most of the code to be shared between the different bubbles in Chrome's Web UI. The CL also fixes several long-standing bugs in the Chrome OS classes options.ManagedNetworkIndicator and options.NetworkList so that the bubbles these pop up behave consistently with the rest. TEST=manual, including UI in rtl languages BUG=104955 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158783

Patch Set 1 #

Patch Set 2 : Rebase and address some of the issues raised offline. #

Patch Set 3 : Reimplemented on top of newly refactored BubbleBase class. #

Total comments: 6

Patch Set 4 : Comment addressed. #

Total comments: 23

Patch Set 5 : All comments addressed. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+521 lines, -292 lines) Patch
M chrome/browser/resources/options/browser_options.css View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/resources/options/chromeos/network_list.js View 1 2 3 4 14 chunks +71 lines, -22 lines 0 comments Download
M chrome/browser/resources/options/controlled_setting.js View 1 2 3 4 2 chunks +124 lines, -93 lines 0 comments Download
M chrome/browser/resources/options/options.html View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
A chrome/browser/resources/options/options_bubble.js View 1 2 3 4 1 chunk +89 lines, -0 lines 0 comments Download
M chrome/browser/resources/options/options_bundle.js View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/resources/options/options_page.css View 1 2 3 4 1 chunk +27 lines, -76 lines 0 comments Download
M chrome/browser/resources/options/options_page.js View 1 2 3 4 3 chunks +39 lines, -0 lines 0 comments Download
M chrome/browser/resources/shared/css/bubble.css View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/shared/js/cr/ui/bubble.js View 1 2 3 4 10 chunks +138 lines, -73 lines 0 comments Download
M chrome/browser/resources/shared/js/cr/ui/focus_manager.js View 1 2 3 4 4 chunks +24 lines, -23 lines 0 comments Download
M chrome/browser/resources/shared_resources.grd View 1 2 2 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
bartfab (slow)
Hi Evan, as discussed, could you please have a look? The ControlledSettingsIndicator used to construct ...
8 years, 3 months ago (2012-09-10 14:29:57 UTC) #1
Evan Stade
have you seen browser/resources/shared/js/cr/ui/bubble.js?
8 years, 3 months ago (2012-09-10 15:18:36 UTC) #2
bartfab (slow)
Yes, of course. However, that class implements quite different behavior, does (intentionally) not capture keyboard ...
8 years, 3 months ago (2012-09-10 15:25:38 UTC) #3
bartfab (slow)
This version is not ready for review yet. I still have to see whether/how I ...
8 years, 3 months ago (2012-09-11 15:34:40 UTC) #4
bartfab (slow)
Hi Evan, Could you have a look at this CL again? I completely reimplemented the ...
8 years, 3 months ago (2012-09-20 16:59:09 UTC) #5
bartfab (slow)
Hi Kevin, I would appreciate it if you could look at this CL as well. ...
8 years, 3 months ago (2012-09-20 17:05:42 UTC) #6
kevers
https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (left): https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css#oldcode466 chrome/browser/resources/options/options_page.css:466: width: 16px; Probably also need "background-size: 16px" here. See ...
8 years, 3 months ago (2012-09-20 17:50:42 UTC) #7
bartfab (slow)
https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (left): https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css#oldcode476 chrome/browser/resources/options/options_page.css:476: background-size: 16px; I tried this out with a Linux ...
8 years, 3 months ago (2012-09-20 18:34:12 UTC) #8
flackr
https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (left): https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css#oldcode476 chrome/browser/resources/options/options_page.css:476: background-size: 16px; On 2012/09/20 18:34:12, bartfab wrote: > I ...
8 years, 3 months ago (2012-09-20 20:00:53 UTC) #9
bartfab (slow)
https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (left): https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css#oldcode476 chrome/browser/resources/options/options_page.css:476: background-size: 16px; Thanks for clarifying - learn something new ...
8 years, 3 months ago (2012-09-20 21:54:17 UTC) #10
flackr
https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (left): https://chromiumcodereview.appspot.com/10907148/diff/11001/chrome/browser/resources/options/options_page.css#oldcode476 chrome/browser/resources/options/options_page.css:476: background-size: 16px; On 2012/09/20 21:54:17, bartfab wrote: > Thanks ...
8 years, 3 months ago (2012-09-20 22:15:25 UTC) #11
Evan Stade
mostly nits https://chromiumcodereview.appspot.com/10907148/diff/17003/chrome/browser/resources/options/bubble.js File chrome/browser/resources/options/bubble.js (right): https://chromiumcodereview.appspot.com/10907148/diff/17003/chrome/browser/resources/options/bubble.js#newcode10 chrome/browser/resources/options/bubble.js:10: Bubble.prototype = { even though they are ...
8 years, 3 months ago (2012-09-21 12:12:11 UTC) #12
bartfab (slow)
https://chromiumcodereview.appspot.com/10907148/diff/17003/chrome/browser/resources/options/bubble.js File chrome/browser/resources/options/bubble.js (right): https://chromiumcodereview.appspot.com/10907148/diff/17003/chrome/browser/resources/options/bubble.js#newcode10 chrome/browser/resources/options/bubble.js:10: Bubble.prototype = { Renamed to OptionsBubble. https://chromiumcodereview.appspot.com/10907148/diff/17003/chrome/browser/resources/options/bubble.js#newcode41 chrome/browser/resources/options/bubble.js:41: * ...
8 years, 2 months ago (2012-09-24 17:15:55 UTC) #13
Evan Stade
lgtm
8 years, 2 months ago (2012-09-25 18:59:26 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/10907148/24001
8 years, 2 months ago (2012-09-26 07:45:11 UTC) #15
commit-bot: I haz the power
8 years, 2 months ago (2012-09-26 11:21:10 UTC) #16
Change committed as 158783

Powered by Google App Engine
This is Rietveld 408576698