Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794763002:
New Sad Tab strings. (Closed)
|
Created:
3 years, 8 months ago by Will Harris Modified:
3 years, 7 months ago Reviewers:
sky CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew Sad Tab strings.
Please see the mocks linked from the bug.
BUG=698368
Review-Url: https://codereview.chromium.org/2794763002
Cr-Commit-Position: refs/heads/master@{#473122}
Committed: https://chromium.googlesource.com/chromium/src/+/ea59f2a232599839afca88f6f71f18a8e5f624de
Patch Set 1 #Patch Set 2 : add bullets #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : right number of spaces as per UI review #
Total comments: 15
Patch Set 5 : add tab count detection. code review changes. #Patch Set 6 : replace spaces with a gridlayout bulletedview #Patch Set 7 : tweak spacing values as per final UI review #Patch Set 8 : final tweak from the denizens of UI review #
Total comments: 2
Patch Set 9 : remove nested view #
Total comments: 4
Patch Set 10 : remove braces #Patch Set 11 : fix compile errors on mac/linux #Patch Set 12 : msvc compile fix #
Messages
Total messages: 41 (14 generated)
Description was changed from ========== New Sad Tab strings. BUG=698368 ========== to ========== New Sad Tab strings. BUG=698368 ==========
wfh@chromium.org changed reviewers: + kylixrd@chromium.org
hi Allen, can you take a look at my feeble attempt to add some views for the new sad tab. in particular I couldn't work out a way to do indenting, and I'm using a unicode bullet (is there another way of getting a bullet). Also, haven't done RTL testing yet. What this CL looks like currently is here -> https://goto.google.com/sad-tab-testing-doc https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:95: bullet_string = L" \u2022 " + bullet_string; I don't know how to do indents here. https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:96: message = CreateLabel(bullet_string); I'm assuming something owns and frees the memory that |message| now points to?
Will need an OWNER for the approval, but these are my initial comments. https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:95: bullet_string = L" \u2022 " + bullet_string; On 2017/04/03 19:49:19, Will Harris (ooo until 10Apr) wrote: > I don't know how to do indents here. Did you try setting the indent padding in layout->StartRowWithPadding()? https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:96: message = CreateLabel(bullet_string); On 2017/04/03 19:49:19, Will Harris (ooo until 10Apr) wrote: > I'm assuming something owns and frees the memory that |message| now points to? The view is "owned" by the SadTabView instance by virtue of also being added to the layout. The SadTabView instance should control its lifetime.
thanks, looking for more advice on how to create the gridlayout or whatever it needs to add the indent, although spaces work fine in both LTR and RTL, I think this should probably be part of the layout. Is there an option in views to show the borders of all the views for debugging? https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:95: bullet_string = L" \u2022 " + bullet_string; On 2017/04/04 20:59:07, kylix_rd wrote: > On 2017/04/03 19:49:19, Will Harris (ooo until 10Apr) wrote: > > I don't know how to do indents here. > > Did you try setting the indent padding in layout->StartRowWithPadding()? which parameter is the indent padding? I saw only options for vertical padding. https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:96: message = CreateLabel(bullet_string); On 2017/04/04 20:59:07, kylix_rd wrote: > On 2017/04/03 19:49:19, Will Harris (ooo until 10Apr) wrote: > > I'm assuming something owns and frees the memory that |message| now points to? > > The view is "owned" by the SadTabView instance by virtue of also being added to > the layout. The SadTabView instance should control its lifetime. "its" refers to the layout? I think the call to SetLayoutManager handles the ownership here.
https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:95: bullet_string = L" \u2022 " + bullet_string; On 2017/04/05 20:01:06, Will Harris (ooo until 10Apr) wrote: > On 2017/04/04 20:59:07, kylix_rd wrote: > > On 2017/04/03 19:49:19, Will Harris (ooo until 10Apr) wrote: > > > I don't know how to do indents here. > > > > Did you try setting the indent padding in layout->StartRowWithPadding()? > > which parameter is the indent padding? I saw only options for vertical padding. Ah, ok. I thought there was an option for indenting. Another way to handle this is to set the insets on the view itself. You may have to create a descendant to override GetInsets(). https://codereview.chromium.org/2794763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:96: message = CreateLabel(bullet_string); On 2017/04/05 20:01:06, Will Harris (ooo until 10Apr) wrote: > On 2017/04/04 20:59:07, kylix_rd wrote: > > On 2017/04/03 19:49:19, Will Harris (ooo until 10Apr) wrote: > > > I'm assuming something owns and frees the memory that |message| now points > to? > > > > The view is "owned" by the SadTabView instance by virtue of also being added > to > > the layout. The SadTabView instance should control its lifetime. > > "its" refers to the layout? > > I think the call to SetLayoutManager handles the ownership here. The layout manager will add the view to the view on which the layout manager is set. Ultimately, each view "owns" it's child views.
wfh@chromium.org changed reviewers: + edwardjung@chromium.org - kylixrd@chromium.org
hi edwardjung - I'm looking for concrete suggestions on how to achieve what I want here, i.e. the indent for the bullet points (rather than using spaces) Do you have some code snippets or ideas for achieving this? Thanks!
On 2017/04/18 17:20:25, Will Harris wrote: > hi edwardjung - I'm looking for concrete suggestions on how to achieve what I > want here, i.e. the indent for the bullet points (rather than using spaces) > > Do you have some code snippets or ideas for achieving this? Thanks! Did a Chrome UX designer do the mocks? I looked at the string spec and it says UX recommends: + Use a single space character between bullet and text, no space in front of the bullet. + When text wraps, indent wrapped text to position of first text character in the bullet's text. So there should be no need for the initial indentation. However if the text does wrap, it needs to be wrapped. So the bullet and text could be formatted in a similar side by side way like the 'learn more' link and 'Reload' button. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/sad_tab_view.cc?...
On 2017/04/19 20:52:46, edwardjung wrote: > On 2017/04/18 17:20:25, Will Harris wrote: > > hi edwardjung - I'm looking for concrete suggestions on how to achieve what I > > want here, i.e. the indent for the bullet points (rather than using spaces) > > > > Do you have some code snippets or ideas for achieving this? Thanks! > > Did a Chrome UX designer do the mocks? I looked at the string spec and it says > UX recommends: The mocks are linked from the doc linked from the launch bug linked from this bug go/better-sad-tab-strings and then click 'here' link for UI flow. > > + Use a single space character between bullet and text, no space in front of the > bullet. > + When text wraps, indent wrapped text to position of first text character in > the bullet's text. > > So there should be no need for the initial indentation. However if the text does > wrap, it needs to be wrapped. So the bullet and text could be formatted in a > similar side by side way like the 'learn more' link and 'Reload' button. I'm just trying to get the output to look like the ones in the mocks. I'm totally unfamiliar with the views code so was looking for some advice on doing this. I'll try trial and error copy/paste other code until it looks right, pkasting also gave me some good hints. > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/sad_tab_view.cc?...
I contacted srahim@ our UX writer and she's getting the final bullet spec and will update the doc.
wfh@chromium.org changed reviewers: - edwardjung@chromium.org
wfh@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== New Sad Tab strings. BUG=698368 ========== to ========== New Sad Tab strings. Please see the mocks linked from the bug. BUG=698368 ==========
On 2017/04/24 09:43:56, edwardjung wrote: > I contacted srahim@ our UX writer and she's getting the final bullet spec and > will update the doc. final bullet spec is http://shortn/_MtZY3KsaZT so I added the required number of spacing. sky -> ptal.
https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... chrome/browser/ui/sad_tab.cc:123: else chromium style guide says no else after return. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... chrome/browser/ui/sad_tab.h:43: int GetBulletText(size_t bullet_id); optional: consider naming this Submessage. While the text does in fact have a bullet in front of it, the name 'bullettext' doesn't really describe what it is. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... chrome/browser/ui/sad_tab.h:61: bool other_tabs_open_; Please add description. AFAICT this is always true. Is it needed? https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:88: // TODO(wfh) add bullet here. Optional: generally chrome code has a ':' after the name, e.g. 'TODO(wfh):', but the style guide allows the format you have here, so I'm saying optional here. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:91: message = new views::Label(bullet_string); The way you have this now, if a line wraps the text is going to line up with the first whitespace character you are using for padding. I would expect text wrapping to line up the first non-bullet/white-space character of the previous line. I recommend using a column for the bullet, then a column for the text. This will result in wrapping like you want. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:158: for (auto* message : messages_) { no {}. optional: auto* -> view::Label* for clarity. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.h (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.h:59: }; DISALLOW...
Patchset #5 (id:80001) has been deleted
Done. I have a question/comment on the spaces... PTAL and give candid opinion :) https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... chrome/browser/ui/sad_tab.cc:123: else On 2017/05/14 13:46:16, sky wrote: > chromium style guide says no else after return. Done. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... chrome/browser/ui/sad_tab.h:43: int GetBulletText(size_t bullet_id); On 2017/05/14 13:46:16, sky wrote: > optional: consider naming this Submessage. While the text does in fact have a > bullet in front of it, the name 'bullettext' doesn't really describe what it is. Done. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/sad_t... chrome/browser/ui/sad_tab.h:61: bool other_tabs_open_; I removed this and now actually detect other tabs being open. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:88: // TODO(wfh) add bullet here. On 2017/05/14 13:46:16, sky wrote: > Optional: generally chrome code has a ':' after the name, e.g. 'TODO(wfh):', but > the style guide allows the format you have here, so I'm saying optional here. I'm not even sure what the TODO means, I think this is all done, so I just removed it. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:91: message = new views::Label(bullet_string); On 2017/05/14 13:46:16, sky wrote: > The way you have this now, if a line wraps the text is going to line up with the > first whitespace character you are using for padding. I would expect text > wrapping to line up the first non-bullet/white-space character of the previous > line. I recommend using a column for the bullet, then a column for the text. > This will result in wrapping like you want. hmm I found however I resized the window I was not able to get this wrapping, but agree that it could happen in theory. The reasons why I went with the 'simple' solution (spaces) were twofold: 1. The UI spec specified the layout using spaces (five before, four after) so I stuck to that. 2. I'm not really too familiar with views code at all, and I don't feel confident trying to work out how to lay this out How strongly do you feel about using a column rather than spaces? if you feel strongly I can try and find some help from a views expert on redesigning this with a column, but I'd probably have to run that past the user interaction designer first (or does this get done in UI implementation review?) Thanks https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:158: for (auto* message : messages_) { On 2017/05/14 13:46:16, sky wrote: > no {}. > optional: auto* -> view::Label* for clarity. Done. https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.h (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.h:59: }; On 2017/05/14 13:46:16, sky wrote: > DISALLOW... Done.
https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:91: message = new views::Label(bullet_string); On 2017/05/17 18:25:19, Will Harris wrote: > On 2017/05/14 13:46:16, sky wrote: > > The way you have this now, if a line wraps the text is going to line up with > the > > first whitespace character you are using for padding. I would expect text > > wrapping to line up the first non-bullet/white-space character of the previous > > line. I recommend using a column for the bullet, then a column for the text. > > This will result in wrapping like you want. > > hmm I found however I resized the window I was not able to get this wrapping, > but agree that it could happen in theory. > > The reasons why I went with the 'simple' solution (spaces) were twofold: > > 1. The UI spec specified the layout using spaces (five before, four after) so I > stuck to that. > 2. I'm not really too familiar with views code at all, and I don't feel > confident trying to work out how to lay this out > > How strongly do you feel about using a column rather than spaces? if you feel > strongly I can try and find some help from a views expert on redesigning this > with a column, but I'd probably have to run that past the user interaction > designer first (or does this get done in UI implementation review?) > > Thanks I would rather see you do it correctly than kick the can down the line. Especially given some locales may have longer strings that trigger wrapping. I also think that while they mentioned a specific number of spacing it's the size that they care about and they could likely tell you what size they want.
On 2017/05/17 19:11:41, sky wrote: > https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... > File chrome/browser/ui/views/sad_tab_view.cc (right): > > https://codereview.chromium.org/2794763002/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/sad_tab_view.cc:91: message = new > views::Label(bullet_string); > On 2017/05/17 18:25:19, Will Harris wrote: > > On 2017/05/14 13:46:16, sky wrote: > > > The way you have this now, if a line wraps the text is going to line up with > > the > > > first whitespace character you are using for padding. I would expect text > > > wrapping to line up the first non-bullet/white-space character of the > previous > > > line. I recommend using a column for the bullet, then a column for the text. > > > This will result in wrapping like you want. > > > > hmm I found however I resized the window I was not able to get this wrapping, > > but agree that it could happen in theory. > > > > The reasons why I went with the 'simple' solution (spaces) were twofold: > > > > 1. The UI spec specified the layout using spaces (five before, four after) so > I > > stuck to that. > > 2. I'm not really too familiar with views code at all, and I don't feel > > confident trying to work out how to lay this out > > > > How strongly do you feel about using a column rather than spaces? if you feel > > strongly I can try and find some help from a views expert on redesigning this > > with a column, but I'd probably have to run that past the user interaction > > designer first (or does this get done in UI implementation review?) > > > > Thanks > > I would rather see you do it correctly than kick the can down the line. > Especially given some locales may have longer strings that trigger wrapping. I > also think that while they mentioned a specific number of spacing it's the size > that they care about and they could likely tell you what size they want. The reason UX specified spaces was "to avoid manual [relative] unit based positioning as that wont translate well to the text APIs we have to work with on iOS and macOS" (from our iOS designer). If it helps, here are the specs In dps: Margin and bullet: 16dp/5 characters Bullet and text: 12dp/4 characters
On 2017/05/17 19:11:41, sky wrote: > I would rather see you do it correctly than kick the can down the line. > Especially given some locales may have longer strings that trigger wrapping. I > also think that while they mentioned a specific number of spacing it's the size > that they care about and they could likely tell you what size they want. okay this is done, there is now a nested GridLayout containing the bullet and the text. PTAL
https://codereview.chromium.org/2794763002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:50: views::ColumnSet* column_set = layout->AddColumnSet(0); Creating another view will certainly work, but GridLayout was designed to support multiple column sets. In other words you shouldn't need to create a view subclass here. Instead add a new columnset to the GridLayout created on line 87 with the config you have here.
And thanks for taking the time to figure it out Will!
https://codereview.chromium.org/2794763002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:50: views::ColumnSet* column_set = layout->AddColumnSet(0); On 2017/05/18 16:10:06, sky wrote: > Creating another view will certainly work, but GridLayout was designed to > support multiple column sets. In other words you shouldn't need to create a view > subclass here. Instead add a new columnset to the GridLayout created on line 87 > with the config you have here. Adding a new column set here means that I need to recreate all the padding to get the rows to align with the previous rows | padding | text | padding | | padding | bul | text | padding | I tried this, so far I'm at http://i.imgur.com/rMuDEdZ.png but it looks like I might need a load of duplication of column layout code of above, I do think the nested view is better here... WDYT?
On 2017/05/18 17:13:29, Will Harris wrote: > https://codereview.chromium.org/2794763002/diff/160001/chrome/browser/ui/view... > File chrome/browser/ui/views/sad_tab_view.cc (right): > > https://codereview.chromium.org/2794763002/diff/160001/chrome/browser/ui/view... > chrome/browser/ui/views/sad_tab_view.cc:50: views::ColumnSet* column_set = > layout->AddColumnSet(0); > On 2017/05/18 16:10:06, sky wrote: > > Creating another view will certainly work, but GridLayout was designed to > > support multiple column sets. In other words you shouldn't need to create a > view > > subclass here. Instead add a new columnset to the GridLayout created on line > 87 > > with the config you have here. > > Adding a new column set here means that I need to recreate all the padding to > get the rows to align with the previous rows > > | padding | text | padding | > | padding | bul | text | padding | > > I tried this, so far I'm at http://i.imgur.com/rMuDEdZ.png but it looks like I > might need a load of duplication of column layout code of above, I do think the > nested view is better here... > > WDYT? Hmm I managed to get the bullet text to align right now, but now when I resize the window it wobbles since it seems it's not pinned right...? My current code is https://gist.github.com/wfharris/c714daf1ab6f467fbd460d0e32ea3bdc
Line 19 in your patch seems unnecessary. You start a row (line 19), and then start another row (line 20). I wouldn't expect that to cause wobble though. You marked both the padding columns (line 6 and line 15) as resizable (the first arg to AddPaddingColumn()). I would expect either to be resizable. Also, you're using constants with the name 'verticalSpacing.' The second argument to AddPaddingColumn is horizontal spacing. Why do you explicitly set the label as disabled? (line 22) That shouldn't matter. On Thu, May 18, 2017 at 10:17 AM, <wfh@chromium.org> wrote: > On 2017/05/18 17:13:29, Will Harris wrote: > > > https://codereview.chromium.org/2794763002/diff/160001/ > chrome/browser/ui/views/sad_tab_view.cc > > File chrome/browser/ui/views/sad_tab_view.cc (right): > > > > > https://codereview.chromium.org/2794763002/diff/160001/ > chrome/browser/ui/views/sad_tab_view.cc#newcode50 > > chrome/browser/ui/views/sad_tab_view.cc:50: views::ColumnSet* > column_set = > > layout->AddColumnSet(0); > > On 2017/05/18 16:10:06, sky wrote: > > > Creating another view will certainly work, but GridLayout was designed > to > > > support multiple column sets. In other words you shouldn't need to > create a > > view > > > subclass here. Instead add a new columnset to the GridLayout created > on line > > 87 > > > with the config you have here. > > > > Adding a new column set here means that I need to recreate all the > padding to > > get the rows to align with the previous rows > > > > | padding | text | padding | > > | padding | bul | text | padding | > > > > I tried this, so far I'm at http://i.imgur.com/rMuDEdZ.png but it looks > like I > > might need a load of duplication of column layout code of above, I do > think > the > > nested view is better here... > > > > WDYT? > > Hmm I managed to get the bullet text to align right now, but now when I > resize > the window it wobbles since it seems it's not pinned right...? > > My current code is > > https://gist.github.com/wfharris/c714daf1ab6f467fbd460d0e32ea3bdc > > https://codereview.chromium.org/2794763002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/18 19:36:22, sky wrote: > Line 19 in your patch seems unnecessary. You start a row (line 19), and > then start another row (line 20). I wouldn't expect that to cause wobble > though. Yes, removing this doesn't change anything. > You marked both the padding columns (line 6 and line 15) as resizable (the > first arg to AddPaddingColumn()). I would expect either to be resizable. I think it's resizable because in SadTabView::Layout() below, it all gets resized... flipping this to 0 puts the layout bound to the left side of the screen. http://i.imgur.com/78SNMIO.png > Also, you're using constants with the name 'verticalSpacing.' The second > argument to AddPaddingColumn is horizontal spacing. yes I noticed that too - bear in mind this code is all copied from above in the code, where it's already being used. > Why do you explicitly set the label as disabled? (line 22) That shouldn't > matter. The label is disabled because it makes the color disabled color (light grey). I'm totally very unfamiliar with views layout, all the code I am taking from cut'n'paste from other places. Are you sure the solution with the nested view is not okay...?
okay this seems to work. thanks for the help! PTAL.
Yay! LGTM https://codereview.chromium.org/2794763002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:41: label->SetEnabled(false); I don't think you need this. https://codereview.chromium.org/2794763002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:178: for (views::Label* label : bullet_labels_) { no {}
thanks for patience while reviewing. https://codereview.chromium.org/2794763002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2794763002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:41: label->SetEnabled(false); On 2017/05/18 20:53:36, sky wrote: > I don't think you need this. this is needed to make the text grey rather than black: http://i.imgur.com/2lK3mrD.png See https://codereview.chromium.org/2838273002 https://codereview.chromium.org/2794763002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:178: for (views::Label* label : bullet_labels_) { On 2017/05/18 20:53:36, sky wrote: > no {} Done.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2794763002/#ps200001 (title: "remove braces")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2794763002/#ps240001 (title: "msvc compile fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495176012586560,
"parent_rev": "e1fa3b55db0f380541270431929002efd8b4ebb5", "commit_rev":
"ea59f2a232599839afca88f6f71f18a8e5f624de"}
Message was sent while issue was closed.
Description was changed from ========== New Sad Tab strings. Please see the mocks linked from the bug. BUG=698368 ========== to ========== New Sad Tab strings. Please see the mocks linked from the bug. BUG=698368 Review-Url: https://codereview.chromium.org/2794763002 Cr-Commit-Position: refs/heads/master@{#473122} Committed: https://chromium.googlesource.com/chromium/src/+/ea59f2a232599839afca88f6f71f... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/ea59f2a232599839afca88f6f71f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
