Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297193002:
Harmony: implement TabbedPane styling (Closed)
|
Created:
4 years, 3 months ago by Elly Fong-Jones Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHarmony: implement TabbedPane styling
This CL:
1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab;
2) Uses those classes to implement the Harmony visuals.
BUG=635176
Committed: https://crrev.com/1968a3e844f82ecac7e14d97747db1a68a96dadf
Cr-Commit-Position: refs/heads/master@{#415734}
Patch Set 1 #
Total comments: 6
Patch Set 2 : style fixes #
Total comments: 4
Messages
Total messages: 17 (6 generated)
Description was changed from ========== Harmony: implement TabbedPane styling This CL: 1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab; 2) Uses those classes to implement the Harmony visuals. BUG=635176 ========== to ========== Harmony: implement TabbedPane styling This CL: 1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab; 2) Uses those classes to implement the Harmony visuals. BUG=635176 ==========
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal? :)
https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:62: // Called whenever tab_state_ changes. Generally we put |'s around references to members/parameters in documentation, e.g. |tab_state_|. See tabbed_pane.h for more examples. https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:63: virtual void OnStateChanged(); Make protected https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:75: Label* title_; Style guide says no protected members.
https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:62: // Called whenever tab_state_ changes. On 2016/08/31 16:44:03, sky wrote: > Generally we put |'s around references to members/parameters in documentation, > e.g. |tab_state_|. See tabbed_pane.h for more examples. Done. https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:63: virtual void OnStateChanged(); On 2016/08/31 16:44:03, sky wrote: > Make protected Done. https://codereview.chromium.org/2297193002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:75: Label* title_; On 2016/08/31 16:44:03, sky wrote: > Style guide says no protected members. Done.
LGTM
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Harmony: implement TabbedPane styling This CL: 1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab; 2) Uses those classes to implement the Harmony visuals. BUG=635176 ========== to ========== Harmony: implement TabbedPane styling This CL: 1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab; 2) Uses those classes to implement the Harmony visuals. BUG=635176 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Harmony: implement TabbedPane styling This CL: 1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab; 2) Uses those classes to implement the Harmony visuals. BUG=635176 ========== to ========== Harmony: implement TabbedPane styling This CL: 1) Adds MdTabStrip and MdTab as subclasses of TabStrip and Tab; 2) Uses those classes to implement the Harmony visuals. BUG=635176 Committed: https://crrev.com/1968a3e844f82ecac7e14d97747db1a68a96dadf Cr-Commit-Position: refs/heads/master@{#415734} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1968a3e844f82ecac7e14d97747db1a68a96dadf Cr-Commit-Position: refs/heads/master@{#415734}
Message was sent while issue was closed.
estade@chromium.org changed reviewers: + estade@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:265: const SkColor kSelectedBorderColor = SkColorSetRGB(0x42, 0x85, 0xF4); Hi Elly, a couple ex post facto comments. Instead of hardcoding these values, they should be drawn from the theme. This one and the selected font one should be kColorId_CallToActionColor. The normal font one should be kColorId_ButtonEnabledColor. The normal border one should be some value that we can share with comboboxes. One practical application (beyond fewer hardcoded values) is that the GTK theme does actually change these values. https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:271: int border_thickness = selected() ? 2 : 1; are you sure these values are right? This results in 2 or 1 dp, but the spec says px. This means at 2x scale (retina) you will get 4px and 2px. Most strokes in harmony are 1px at all scale factors so I suspect this is the same. https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:281: font_weight = gfx::Font::Weight::BOLD; should this not just be MEDIUM on all platforms?
Message was sent while issue was closed.
https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:265: const SkColor kSelectedBorderColor = SkColorSetRGB(0x42, 0x85, 0xF4); On 2016/09/01 20:46:07, Evan Stade wrote: > Hi Elly, a couple ex post facto comments. Instead of hardcoding these values, > they should be drawn from the theme. This one and the selected font one should > be kColorId_CallToActionColor. The normal font one should be > kColorId_ButtonEnabledColor. The normal border one should be some value that we > can share with comboboxes. > > One practical application (beyond fewer hardcoded values) is that the GTK theme > does actually change these values. I'm thinking the border should use kColorId_FocusedBorderColor/kColorId_UnfocusedBorderColor (although currently the latter value is wrong and would need to be updated).
Message was sent while issue was closed.
On 2016/09/01 20:46:07, Evan Stade (ooo friday) wrote: > https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): > > https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > ui/views/controls/tabbed_pane/tabbed_pane.cc:265: const SkColor > kSelectedBorderColor = SkColorSetRGB(0x42, 0x85, 0xF4); > Hi Elly, a couple ex post facto comments. Instead of hardcoding these values, > they should be drawn from the theme. This one and the selected font one should > be kColorId_CallToActionColor. The normal font one should be > kColorId_ButtonEnabledColor. The normal border one should be some value that we > can share with comboboxes. > > One practical application (beyond fewer hardcoded values) is that the GTK theme > does actually change these values. Done: https://codereview.chromium.org/2307843002/ > https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > ui/views/controls/tabbed_pane/tabbed_pane.cc:271: int border_thickness = > selected() ? 2 : 1; > are you sure these values are right? This results in 2 or 1 dp, but the spec > says px. This means at 2x scale (retina) you will get 4px and 2px. Most strokes > in harmony are 1px at all scale factors so I suspect this is the same. I don't understand - Views appears to be full of "thicknesses" that are simply defined as 1 or whatever and not scaled/etc before being used. For example, https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button.... just uses "1" as a thickness regardless of device scale factor. Is there some other scaling that I need to do here to turn the 2/1 into actual physical pixels? If so, how come it is not done all over Views? > https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > ui/views/controls/tabbed_pane/tabbed_pane.cc:281: font_weight = > gfx::Font::Weight::BOLD; > should this not just be MEDIUM on all platforms? The Harmony spec I'm working off of (https://goto.google.com/cr-harmony-specs), on "SPEC-secondary-UI-07-tabs", says "Font: system Medium, bold(win)" for the Selected state.
Message was sent while issue was closed.
> https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > > ui/views/controls/tabbed_pane/tabbed_pane.cc:265: const SkColor > > kSelectedBorderColor = SkColorSetRGB(0x42, 0x85, 0xF4); > > Hi Elly, a couple ex post facto comments. Instead of hardcoding these values, > > they should be drawn from the theme. This one and the selected font one should > > be kColorId_CallToActionColor. The normal font one should be > > kColorId_ButtonEnabledColor. The normal border one should be some value that > we > > can share with comboboxes. > > > > One practical application (beyond fewer hardcoded values) is that the GTK > theme > > does actually change these values. > > Done: https://codereview.chromium.org/2307843002/ > > > > https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > > ui/views/controls/tabbed_pane/tabbed_pane.cc:271: int border_thickness = > > selected() ? 2 : 1; > > are you sure these values are right? This results in 2 or 1 dp, but the spec > > says px. This means at 2x scale (retina) you will get 4px and 2px. Most > strokes > > in harmony are 1px at all scale factors so I suspect this is the same. > > I don't understand - Views appears to be full of "thicknesses" that are simply > defined as 1 or whatever and not scaled/etc before being used. For example, > https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button.... > just uses "1" as a thickness regardless of device scale factor. Is there some > other scaling that I need to do here to turn the 2/1 into actual physical > pixels? If so, how come it is not done all over Views? The focus ring is defined in dp, so no special work is needed. Here's an example of a 1px stroke: https://cs.chromium.org/chromium/src/ui/views/controls/focusable_border.cc?rc... > > > > https://codereview.chromium.org/2297193002/diff/20001/ui/views/controls/tabbe... > > ui/views/controls/tabbed_pane/tabbed_pane.cc:281: font_weight = > > gfx::Font::Weight::BOLD; > > should this not just be MEDIUM on all platforms? > > The Harmony spec I'm working off of (https://goto.google.com/cr-harmony-specs), > on "SPEC-secondary-UI-07-tabs", says "Font: system Medium, bold(win)" for the > Selected state. OK, but medium is not the same as normal. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
