Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10867021:
NTP5: Improving the Tile Page resize logic (Closed)
|
Created:
8 years, 4 months ago by pedrosimonetti2 Modified:
8 years, 3 months ago CC:
chromium-reviews, arv (Not doing code reviews), mwichary_google.com, dmazzoni, Glen Murphy Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionNTP5: Improving the Tile Page resize logic.
- Added constants for resize-related logic
- Tweaked constant values in order to play nicer with the constrained size of he Bottom Section WebView
- Added a show/hide Bottom Section animation
BUG=142669
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154860
Patch Set 1 #
Total comments: 33
Patch Set 2 : Addressing Jeremy's comments #
Total comments: 4
Patch Set 3 : Rebasing and addressing Jeremy's comments #Patch Set 4 : Improving comments #
Total comments: 8
Patch Set 5 : Addressing Dan's comments #
Total comments: 1
Patch Set 6 : Addressing Dan's comment #Patch Set 7 : Fixing ident problem #
Total comments: 8
Patch Set 8 : Addressing Evan's comments #Patch Set 9 : Addressing Evan's comments #
Total comments: 2
Patch Set 10 : Addressing Dan's comment #Patch Set 11 : Resync'ing and rebasing #
Messages
Total messages: 27 (0 generated)
Demo looks good! https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/new_tab.css (right): https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/new_tab.css:13: font-family: Arial !important; Is this necessary? I don't see font-family being overridden anywhere. https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/tile_page.js (right): https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:13: * The maximum window height required to show 2 rows of Tiles in the Bottom minimum https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:18: var MAX_WINDOW_HEIGHT = 275; To me, this sounds like the window can't be larger than this value. Maybe TWO_ROW_WINDOW_HEIGHT or TWO_ROW_REQUIRED_HEIGHT or HEIGHT_FOR_TWO_ROWS? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:26: var MIN_WINDOW_HEIGHT = 170; See above comment, maybe BOTTOM_SECTION_WINDOW_HEIGHT, etc.? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:29: * The maximum window width required to show 5 cols of Tiles in the Bottom minimum https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:30: * Section. If the width is bigger than this value, 5 cols will be displayed. Shouldn't this be used in getColCountForWidth_ then? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:34: var MAX_WINDOW_WIDTH = 948; Maybe LARGE_WINDOW_WIDTH? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:34: var MAX_WINDOW_WIDTH = 948; This value is also used in the bottom section width computation - can you include a comment for this? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:38: * this value, then the Bottom Section width will be the window width minus Stick with either Bottom Section or Bottom Panel everywhere. https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:54: var MIN_WINDOW_WIDTH = 300; Maybe SMALL_WINDOW_WIDTH? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:577: else if (windowWidth >= MIN_WINDOW_WIDTH) Add curlies, since this is multi-line? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:709: var numOfVisibleRows = windowHeight > MAX_WINDOW_HEIGHT ? 2 : 1; When windowHeight < MIN_WINDOW_HEIGHT, should numOfVisibleRows be 0? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:771: showBottomSection_: function(show) { Shouldn't the parameter be 'hide'? Maybe make this animateBottomSection_ instead? https://chromiumcodereview.appspot.com/10867021/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:772: $('card-slider-frame'). nit: I think it's more typical to line-break on a '(' instead.
Hey Jeremy, thanks for your comments. I tried to come up with more descriptive names for the constants. It's kinda hard because the resizing logic is complex and difficult to verbalize, but I think the names are more meaningful now. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/new_tab.css:13: font-family: Arial !important; On 2012/08/23 01:09:28, jeremycho wrote: > Is this necessary? I don't see font-family being overridden anywhere. The font-family and size is being set using JavaScript (based on values that are in loadTimeData), so the only way to override is to use !important. This is just a temporary fix. We need to talk to UX folks to confirm we want the same font on all platforms (which I'm 90% sure is what they want) and we need to confirm with Evan which is the best place to put this change. Maybe in th C++ code where the values that are passed to loadTimeData are defined. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:13: * The maximum window height required to show 2 rows of Tiles in the Bottom On 2012/08/23 01:09:28, jeremycho wrote: > minimum Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:18: var MAX_WINDOW_HEIGHT = 275; On 2012/08/23 01:09:28, jeremycho wrote: > To me, this sounds like the window can't be larger than this value. Maybe > TWO_ROW_WINDOW_HEIGHT or TWO_ROW_REQUIRED_HEIGHT or HEIGHT_FOR_TWO_ROWS? Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:26: var MIN_WINDOW_HEIGHT = 170; On 2012/08/23 01:09:28, jeremycho wrote: > See above comment, maybe BOTTOM_SECTION_WINDOW_HEIGHT, etc.? Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:29: * The maximum window width required to show 5 cols of Tiles in the Bottom On 2012/08/23 01:09:28, jeremycho wrote: > minimum Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:30: * Section. If the width is bigger than this value, 5 cols will be displayed. On 2012/08/23 01:09:28, jeremycho wrote: > Shouldn't this be used in getColCountForWidth_ then? I don't think so because getColCountForWidth_ will only receive number of columns that can fit in the max width defined here. So, it will never be higher than 5 in our case. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:34: var MAX_WINDOW_WIDTH = 948; On 2012/08/23 01:09:28, jeremycho wrote: > Maybe LARGE_WINDOW_WIDTH? Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:34: var MAX_WINDOW_WIDTH = 948; On 2012/08/23 01:09:28, jeremycho wrote: > This value is also used in the bottom section width computation - can you > include a comment for this? I'm not sure if I understand what you mean by "this value is *also* used in the bottom section width computation". This value is *just* being used for width computation. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:38: * this value, then the Bottom Section width will be the window width minus On 2012/08/23 01:09:28, jeremycho wrote: > Stick with either Bottom Section or Bottom Panel everywhere. Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:54: var MIN_WINDOW_WIDTH = 300; On 2012/08/23 01:09:28, jeremycho wrote: > Maybe SMALL_WINDOW_WIDTH? Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:577: else if (windowWidth >= MIN_WINDOW_WIDTH) On 2012/08/23 01:09:28, jeremycho wrote: > Add curlies, since this is multi-line? Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:709: var numOfVisibleRows = windowHeight > MAX_WINDOW_HEIGHT ? 2 : 1; On 2012/08/23 01:09:28, jeremycho wrote: > When windowHeight < MIN_WINDOW_HEIGHT, should numOfVisibleRows be 0? I thought about this too. Even though zero rows are 'visible' when the entire Bottom Panel is hidden, what is actually being hidden is the panel, but the rows themselves remain the way they were before. In other words, numOfVisibleRows is tracking the number of rows that are being hidden with CSS. When we hide the Bottom Panel, just the Panel is hidden with CSS, but the row remains 'visible'. It just turns out that you can't see it ;) http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:771: showBottomSection_: function(show) { On 2012/08/23 01:09:28, jeremycho wrote: > Shouldn't the parameter be 'hide'? Maybe make this animateBottomSection_ > instead? I changed the implementation so it uses a show param, because in order to keep the code consistent with other methods like showTileRow_() and showTileCols_(). http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:772: $('card-slider-frame'). On 2012/08/23 01:09:28, jeremycho wrote: > nit: I think it's more typical to line-break on a '(' instead. Done.
Thanks, much clearer now. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/new_tab.css:13: font-family: Arial !important; Ah now I see where this happens. Sounds like C++ might be the right place, but I'm fine with doing that later. On 2012/08/24 00:22:15, pedrosimonetti wrote: > On 2012/08/23 01:09:28, jeremycho wrote: > > Is this necessary? I don't see font-family being overridden anywhere. > > The font-family and size is being set using JavaScript (based on values that are > in loadTimeData), so the only way to override is to use !important. This is just > a temporary fix. We need to talk to UX folks to confirm we want the same font on > all platforms (which I'm 90% sure is what they want) and we need to confirm with > Evan which is the best place to put this change. Maybe in th C++ code where the > values that are passed to loadTimeData are defined. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:34: var MAX_WINDOW_WIDTH = 948; Not done? On 2012/08/24 00:22:15, pedrosimonetti wrote: > On 2012/08/23 01:09:28, jeremycho wrote: > > Maybe LARGE_WINDOW_WIDTH? > > Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:34: var MAX_WINDOW_WIDTH = 948; Sorry, what I meant was - can you give a comment explaining what it does in the width computation? Seems it should be analogous to the comment for MIN_BOTTOM_PANEL_WIDTH. On 2012/08/24 00:22:15, pedrosimonetti wrote: > On 2012/08/23 01:09:28, jeremycho wrote: > > This value is also used in the bottom section width computation - can you > > include a comment for this? > > I'm not sure if I understand what you mean by "this value is *also* used in the > bottom section width computation". This value is *just* being used for width > computation. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:54: var MIN_WINDOW_WIDTH = 300; Not done? On 2012/08/24 00:22:15, pedrosimonetti wrote: > On 2012/08/23 01:09:28, jeremycho wrote: > > Maybe SMALL_WINDOW_WIDTH? > > Done. http://codereview.chromium.org/10867021/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:709: var numOfVisibleRows = windowHeight > MAX_WINDOW_HEIGHT ? 2 : 1; Ok, makes sense. On 2012/08/24 00:22:15, pedrosimonetti wrote: > On 2012/08/23 01:09:28, jeremycho wrote: > > When windowHeight < MIN_WINDOW_HEIGHT, should numOfVisibleRows be 0? > > I thought about this too. Even though zero rows are 'visible' when the entire > Bottom Panel is hidden, what is actually being hidden is the panel, but the rows > themselves remain the way they were before. > > In other words, numOfVisibleRows is tracking the number of rows that are being > hidden with CSS. When we hide the Bottom Panel, just the Panel is hidden with > CSS, but the row remains 'visible'. It just turns out that you can't see it ;) http://codereview.chromium.org/10867021/diff/4001/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10867021/diff/4001/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:20: * The minimum height required to show the Bottom Panel. If the height is nit:The second sentence seems redundant. And I think you can drop 'minimum' to match the other comments. http://codereview.chromium.org/10867021/diff/4001/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:47: * The minimum Bottom Panel width. If the available width is smaller than To me, this first sentence is misleading, since the width can actually be smaller than this.
http://codereview.chromium.org/10867021/diff/4001/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10867021/diff/4001/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:20: * The minimum height required to show the Bottom Panel. If the height is On 2012/08/24 00:54:42, jeremycho wrote: > nit:The second sentence seems redundant. And I think you can drop 'minimum' to > match the other comments. Done. http://codereview.chromium.org/10867021/diff/4001/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:47: * The minimum Bottom Panel width. If the available width is smaller than On 2012/08/24 00:54:42, jeremycho wrote: > To me, this first sentence is misleading, since the width can actually be > smaller than this. I think it's better now.
lgtm
Thanks Jeremy! Hi Evan, we're getting there! This is our last CL we have planned for dogfood! Yay!
I'll have to defer this one to Dan as I'm not back in the office till Monday.
https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... File chrome/browser/resources/ntp_search/tile_page.js (right): https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:14: * @type {Number} @type {number} (number is when you say 5, Number is when you say new Number(5)) https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:32: var MAX_BOTTOM_PANEL_WIDTH = 948; why can't these be in CSS as max-width, min-width? https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:537: if (windowWidth >= MAX_BOTTOM_PANEL_WIDTH) nit: curlies when any part of the if/else/else if chain has curlies https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:543: width = Math.floor((windowWidth - MIN_BOTTOM_PANEL_WIDTH) / can you use some temporary variables to help document these subexpressions?
https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... File chrome/browser/resources/ntp_search/tile_page.js (right): https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:14: * @type {Number} On 2012/08/25 00:13:29, Dan Beam wrote: > @type {number} (number is when you say 5, Number is when you say new Number(5)) Done. https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:32: var MAX_BOTTOM_PANEL_WIDTH = 948; On 2012/08/25 00:13:29, Dan Beam wrote: > why can't these be in CSS as max-width, min-width? Even though I could implement this particularly with CSS (max width), the entire resizing logic is not possible to be implemented using CSS only. Because of that I think it's better having all logic in once place, instead of having it partially implemented with CSS and partially with JavaScript. https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:537: if (windowWidth >= MAX_BOTTOM_PANEL_WIDTH) On 2012/08/25 00:13:29, Dan Beam wrote: > nit: curlies when any part of the if/else/else if chain has curlies Done. https://chromiumcodereview.appspot.com/10867021/diff/2003/chrome/browser/reso... chrome/browser/resources/ntp_search/tile_page.js:543: width = Math.floor((windowWidth - MIN_BOTTOM_PANEL_WIDTH) / On 2012/08/25 00:13:29, Dan Beam wrote: > can you use some temporary variables to help document these subexpressions? Done. I reviewed the entire expression again, and included some temporary variables.
http://codereview.chromium.org/10867021/diff/8003/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/8003/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/new_tab.css:13: font-family: Arial !important; remove this !important. setting an !important on a very low specificity place like <tagname> on something as global as body is a very bad idea.
Me and Dan discussed offline the use of !important to override the font-family a little bit. The solution I found was setting the font-family on the card-slider-frame.
lgtm pending dan's review
http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:19: /* TODO(pedrosimonetti): Confirm with designers/engineers what do we want nit: missing an asterisk here /* Blah blah blah, look how cool this message is. * Man I love writing stuff in CSS. */ ^ http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; so this will not apply to NTP4, correct?
http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:19: /* TODO(pedrosimonetti): Confirm with designers/engineers what do we want On 2012/08/28 22:55:34, Dan Beam wrote: > nit: missing an asterisk here > > /* Blah blah blah, look how cool this message is. > * Man I love writing stuff in CSS. */ > ^ Done. I was unsure whether or not the asterisk should be used for non-JsDoc-style comments. http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; On 2012/08/28 22:55:34, Dan Beam wrote: > so this will not apply to NTP4, correct? Correct. This stylesheet lives on the ntp_search directory and is only loaded in the NTP5, so it does not interfere in the NTP4 behavior.
http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; On 2012/08/28 23:04:44, pedrosimonetti wrote: > On 2012/08/28 22:55:34, Dan Beam wrote: > > so this will not apply to NTP4, correct? > > Correct. This stylesheet lives on the ntp_search directory and is only loaded in > the NTP5, so it does not interfere in the NTP4 behavior. I don't think that you want Arial here. We had a long design discussion in the past to conclude that platform-specific fonts should be used. What has changed? That said, if you do insist on arial, then you should put it on body and just remove .style.fontFamily:fontfamily from the body tag in new_tab.html.
On 2012/08/28 23:08:24, Evan Stade wrote: > http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... > File chrome/browser/resources/ntp_search/new_tab.css (right): > > http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; > On 2012/08/28 23:04:44, pedrosimonetti wrote: > > On 2012/08/28 22:55:34, Dan Beam wrote: > > > so this will not apply to NTP4, correct? > > > > Correct. This stylesheet lives on the ntp_search directory and is only loaded > in > > the NTP5, so it does not interfere in the NTP4 behavior. > > I don't think that you want Arial here. We had a long design discussion in the > past to conclude that platform-specific fonts should be used. What has changed? > > That said, if you do insist on arial, then you should put it on body and just > remove .style.fontFamily:fontfamily from the body tag in new_tab.html. I agree with estade@, please resolve this first.
Dan and Evan, please take a look at my latest Patch List. http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; On 2012/08/28 23:08:24, Evan Stade wrote: > On 2012/08/28 23:04:44, pedrosimonetti wrote: > > On 2012/08/28 22:55:34, Dan Beam wrote: > > > so this will not apply to NTP4, correct? > > > > Correct. This stylesheet lives on the ntp_search directory and is only loaded > in > > the NTP5, so it does not interfere in the NTP4 behavior. > > I don't think that you want Arial here. We had a long design discussion in the > past to conclude that platform-specific fonts should be used. What has changed? I thought we had a different conclusion. I thought the problem was using !important, not using Arial per se. I just confirmed with Marcin (mwichary@), one the designers leading our UX effort, and he said that we do want to use Arial for all platforms, and of course, the UI Leads are aware of this change. We're trying to provide a unifying experience so we're going to use the same font on all platforms, and on all (or at least the most important) UI elements such the Omnibox and Suggestions. > > That said, if you do insist on arial, then you should put it on body and just > remove .style.fontFamily:fontfamily from the body tag in new_tab.html. Okay, that seems reasonable. I just did that.
You're going to use Arial as the default or even after a user has explicitly changed their font in settings? http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.html:86: <body id="ntp5" nit: unwrap
That's a good question. I need to ask the UX folks what happens when the user changes the font settings, because AFAIK we don't have a clear plan for this case. I added a note in our design doc, I'll discuss this issue with the UX folks and engineers in our next meeting. http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.html:86: <body id="ntp5" On 2012/08/29 00:16:02, Dan Beam wrote: > nit: unwrap Done.
David and I just clarified that font overrides don’t really affect NTP/DOMUI pages in Chrome stable today, so outside of font size changes for accessibility this shouldn’t be any worry for us. On Tue, Aug 28, 2012 at 5:33 PM, <pedrosimonetti@google.com> wrote: > That's a good question. I need to ask the UX folks what happens when the > user > changes the font settings, because AFAIK we don't have a clear plan for > this > case. > > I added a note in our design doc, I'll discuss this issue with the UX > folks and > engineers in our next meeting. > > > > http://codereview.chromium.**org/10867021/diff/16003/** > chrome/browser/resources/ntp_**search/new_tab.html<http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/ntp_search/new_tab.html> > File chrome/browser/resources/ntp_**search/new_tab.html (right): > > http://codereview.chromium.**org/10867021/diff/16003/** > chrome/browser/resources/ntp_**search/new_tab.html#newcode86<http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/ntp_search/new_tab.html#newcode86> > chrome/browser/resources/ntp_**search/new_tab.html:86: <body id="ntp5" > On 2012/08/29 00:16:02, Dan Beam wrote: > >> nit: unwrap >> > > Done. > > http://codereview.chromium.**org/10867021/<http://codereview.chromium.org/108... >
lgtm after discovering we don't actually let users set the font on webui pages :( and talking with mwichary more, sorry this took a little while
Sorry, I meant Dan, not David. On Wed, Aug 29, 2012 at 12:04 PM, Marcin Wichary <mwichary@google.com>wrote: > David and I just clarified that font overrides don’t really affect > NTP/DOMUI pages in Chrome stable today, so outside of font size changes for > accessibility this shouldn’t be any worry for us. > > > On Tue, Aug 28, 2012 at 5:33 PM, <pedrosimonetti@google.com> wrote: > >> That's a good question. I need to ask the UX folks what happens when the >> user >> changes the font settings, because AFAIK we don't have a clear plan for >> this >> case. >> >> I added a note in our design doc, I'll discuss this issue with the UX >> folks and >> engineers in our next meeting. >> >> >> >> http://codereview.chromium.**org/10867021/diff/16003/** >> chrome/browser/resources/ntp_**search/new_tab.html<http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/ntp_search/new_tab.html> >> File chrome/browser/resources/ntp_**search/new_tab.html (right): >> >> http://codereview.chromium.**org/10867021/diff/16003/** >> chrome/browser/resources/ntp_**search/new_tab.html#newcode86<http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/ntp_search/new_tab.html#newcode86> >> chrome/browser/resources/ntp_**search/new_tab.html:86: <body id="ntp5" >> On 2012/08/29 00:16:02, Dan Beam wrote: >> >>> nit: unwrap >>> >> >> Done. >> >> http://codereview.chromium.**org/10867021/<http://codereview.chromium.org/108... >> > >
Thanks Dan and Marcin, I gotta go catch the shuttle to Lake Tahoe now, so I'll submit this next week, specially because I think there's gonna be one conflict with the CL Jeremy is trying to submit right now (Favicon + Thumbnail Stack CL). Pedro Simonetti Garcia. On Wed, Aug 29, 2012 at 12:07 PM, Marcin Wichary <mwichary@google.com>wrote: > Sorry, I meant Dan, not David. > > > On Wed, Aug 29, 2012 at 12:04 PM, Marcin Wichary <mwichary@google.com>wrote: > >> David and I just clarified that font overrides don’t really affect >> NTP/DOMUI pages in Chrome stable today, so outside of font size changes for >> accessibility this shouldn’t be any worry for us. >> >> >> On Tue, Aug 28, 2012 at 5:33 PM, <pedrosimonetti@google.com> wrote: >> >>> That's a good question. I need to ask the UX folks what happens when the >>> user >>> changes the font settings, because AFAIK we don't have a clear plan for >>> this >>> case. >>> >>> I added a note in our design doc, I'll discuss this issue with the UX >>> folks and >>> engineers in our next meeting. >>> >>> >>> >>> http://codereview.chromium.**org/10867021/diff/16003/** >>> chrome/browser/resources/ntp_**search/new_tab.html<http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/ntp_search/new_tab.html> >>> File chrome/browser/resources/ntp_**search/new_tab.html (right): >>> >>> http://codereview.chromium.**org/10867021/diff/16003/** >>> chrome/browser/resources/ntp_**search/new_tab.html#newcode86<http://codereview.chromium.org/10867021/diff/16003/chrome/browser/resources/ntp_search/new_tab.html#newcode86> >>> chrome/browser/resources/ntp_**search/new_tab.html:86: <body id="ntp5" >>> On 2012/08/29 00:16:02, Dan Beam wrote: >>> >>>> nit: unwrap >>>> >>> >>> Done. >>> >>> http://codereview.chromium.**org/10867021/<http://codereview.chromium.org/108... >>> >> >> >
http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; On 2012/08/28 23:47:28, pedrosimonetti wrote: > On 2012/08/28 23:08:24, Evan Stade wrote: > > On 2012/08/28 23:04:44, pedrosimonetti wrote: > > > On 2012/08/28 22:55:34, Dan Beam wrote: > > > > so this will not apply to NTP4, correct? > > > > > > Correct. This stylesheet lives on the ntp_search directory and is only > loaded > > in > > > the NTP5, so it does not interfere in the NTP4 behavior. > > > > I don't think that you want Arial here. We had a long design discussion in the > > past to conclude that platform-specific fonts should be used. What has > changed? > > I thought we had a different conclusion. I thought the problem was using > !important, not using Arial per se. > > I just confirmed with Marcin (mwichary@), one the designers leading our UX > effort, and he said that we do want to use Arial for all platforms, and of > course, the UI Leads are aware of this change. We're trying to provide a > unifying experience so we're going to use the same font on all platforms, and on > all (or at least the most important) UI elements such the Omnibox and > Suggestions. was there any discussion of what makes ntp5 different in this regard from ntp4, and every other webui page in existence? > > > > > That said, if you do insist on arial, then you should put it on body and just > > remove .style.fontFamily:fontfamily from the body tag in new_tab.html. > > Okay, that seems reasonable. I just did that.
http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10867021/diff/21001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:21: font-family: Arial; On 2012/08/28 23:47:28, pedrosimonetti wrote: > On 2012/08/28 23:08:24, Evan Stade wrote: > > On 2012/08/28 23:04:44, pedrosimonetti wrote: > > > On 2012/08/28 22:55:34, Dan Beam wrote: > > > > so this will not apply to NTP4, correct? > > > > > > Correct. This stylesheet lives on the ntp_search directory and is only > loaded > > in > > > the NTP5, so it does not interfere in the NTP4 behavior. > > > > I don't think that you want Arial here. We had a long design discussion in the > > past to conclude that platform-specific fonts should be used. What has > changed? > > I thought we had a different conclusion. I thought the problem was using > !important, not using Arial per se. > > I just confirmed with Marcin (mwichary@), one the designers leading our UX > effort, and he said that we do want to use Arial for all platforms, and of > course, the UI Leads are aware of this change. We're trying to provide a > unifying experience so we're going to use the same font on all platforms, and on > all (or at least the most important) UI elements such the Omnibox and > Suggestions. note that the native omnibox and suggestions also use platform-specific fonts and do not hardcode arial. > > > > > That said, if you do insist on arial, then you should put it on body and just > > remove .style.fontFamily:fontfamily from the body tag in new_tab.html. > > Okay, that seems reasonable. I just did that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@google.com/10867021/30001
Change committed as 154860 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
