|
|
Created:
7 years, 11 months ago by dconnelly Modified:
7 years, 10 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews), pedrosimonetti+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLayout the NTP footer with flexboxes.
This updates the CSS flexbox property names to their most recent
iterations, displays the entire footer in a flexbox to make the layout
more obvious, and rearranges the footer's right-side flexbox so that it
always has the same width.
The logo on the footer's left side is resized once when the page is
rendered to make it the same width as the right side. This keeps the
nav dots centered in the footer.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182190
Patch Set 1 #
Total comments: 21
Patch Set 2 : Clean up CSS per review suggestions #Patch Set 3 : classList#toggle #
Total comments: 2
Patch Set 4 : move separator out of webstore link #
Total comments: 3
Patch Set 5 : rebase #
Messages
Total messages: 36 (0 generated)
Hi James, I had Evan as a reviewer on a couple of CLs, but it now appears that he is out of the office. Some of the other Chrome Enterprise guys suggested that I add you as a reviewer. Could you look at this CL (and a forthcoming additional CL), or suggest someone else? Thanks!
On 2013/01/25 15:57:40, dconnelly wrote: > Hi James, > > I had Evan as a reviewer on a couple of CLs, but it now appears that he is out > of the office. Some of the other Chrome Enterprise guys suggested that I add > you as a reviewer. Could you look at this CL (and a forthcoming additional CL), > or suggest someone else? > > Thanks! I'm not an OWNER of ntp4. I suggest pinging Evan again, or hit up dbeam@ (or rbyers I suppose).
By out of office, I mean that he's on vacation auto-responder that says "return date unsure." I'll ask dbeam. Thanks! On Fri, Jan 25, 2013 at 6:14 PM, <jhawkins@chromium.org> wrote: > On 2013/01/25 15:57:40, dconnelly wrote: >> >> Hi James, > > >> I had Evan as a reviewer on a couple of CLs, but it now appears that he is >> out >> of the office. Some of the other Chrome Enterprise guys suggested that I >> add >> you as a reviewer. Could you look at this CL (and a forthcoming >> additional > > CL), >> >> or suggest someone else? > > >> Thanks! > > > I'm not an OWNER of ntp4. I suggest pinging Evan again, or hit up dbeam@ > (or > rbyers I suppose). > > https://codereview.chromium.org/12038090/
so we already did this once or twice (used new display: [-webkit-]flex;) and it kept breaking because they'd change property names every week. is there any evidence this is done yet?
for example: https://chromiumcodereview.appspot.com/10332274
On 2013/01/25 20:17:53, Dan Beam wrote: > for example: https://chromiumcodereview.appspot.com/10332274 The flexbox standard has moved from "Working Draft" status to "Candidate Recommendation", which apparently means: "the standard document may change further, but at this point, significant features are mostly locked. The design of those features can still change due to feedback from implementors." So, it's possible it may change, but it's probably pretty safe now.
So, given this information, does anybody feel like doing a review? On Mon, Jan 28, 2013 at 9:14 AM, <dubroy@chromium.org> wrote: > On 2013/01/25 20:17:53, Dan Beam wrote: >> >> for example: https://chromiumcodereview.appspot.com/10332274 > > > The flexbox standard has moved from "Working Draft" status to "Candidate > Recommendation", which apparently means: "the standard document may change > further, but at this point, significant features are mostly locked. The > design > of those features can still change due to feedback from implementors." > > So, it's possible it may change, but it's probably pretty safe now. > > https://codereview.chromium.org/12038090/
yes, I can review. have you been testing the keyboard tab order while developing this, by chance? I'm not sure that -webkit-order accounts for this (but haven't checked), so if you re-arrange some nodes with it visually the tab order will be strange looking (happened in the past as well).
Good news! The keyboard tab order appears to be working properly. I can sign in and back out so the "Other Sessions" button appears/disappears, close a tab so the "Recently Closed" button appears, and the keyboard tab selection skips the invisible elements and goes left-to-right across whichever buttons are visible. I'm not sure if that's exactly what you're looking for, so let me know. On Mon, Jan 28, 2013 at 10:46 PM, <dbeam@chromium.org> wrote: > yes, I can review. have you been testing the keyboard tab order while > developing this, by chance? I'm not sure that -webkit-order accounts for > this > (but haven't checked), so if you re-arrange some nodes with it visually the > tab > order will be strange looking (happened in the past as well). > > https://codereview.chromium.org/12038090/
Ping On Tue, Jan 29, 2013 at 3:37 PM, Daniel Connelly <dconnelly@google.com> wrote: > Good news! The keyboard tab order appears to be working properly. I > can sign in and back out so the "Other Sessions" button > appears/disappears, close a tab so the "Recently Closed" button > appears, and the keyboard tab selection skips the invisible elements > and goes left-to-right across whichever buttons are visible. I'm not > sure if that's exactly what you're looking for, so let me know. > > On Mon, Jan 28, 2013 at 10:46 PM, <dbeam@chromium.org> wrote: >> yes, I can review. have you been testing the keyboard tab order while >> developing this, by chance? I'm not sure that -webkit-order accounts for >> this >> (but haven't checked), so if you re-arrange some nodes with it visually the >> tab >> order will be strange looking (happened in the past as well). >> >> https://codereview.chromium.org/12038090/
sorry, was in MTV, looking now
https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.css (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:184: -webkit-flex-direction: row; this seems to be the default value (so probably not needed) https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:185: -webkit-justify-content: space-between; why is -webkit-justify-content needed? https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:437: .footer-menu-button:not(.invisible) ~ #chrome-web-store-link > #vertical-separator { 80 col wrap, > isn't needed https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:458: #recently-closed-menu-button.invisible { why did you bring back this whole .invisible business? IIRC it messes up the centering of the nav "dots" ([ Most visited ][ Apps ] thingies)? https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:467: #other-sessions-menu-button.invisible { combine with selector above, i.e. #chrome-web-store-link.invisible, #recently-closed-menu-button.invisible, #other-sessions-menu-button.invisible { -webkit-order: -1; visibility: hidden; } or better yet #footer-menu-container .invisible { -webkit-order: -1; visibility: hidden; } https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:472: #other-sessions-menu-button::not(.invisible) { ::not() should be :not() (:: means psuedo-element, : means psuedo-selector) https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.html:118: <a id="chrome-web-store-link"> this is not a menu so it doesn't belong here https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.js:345: logo.style.width = menu.clientWidth + 'px'; this shouldn't be necessary and can be done in CSS https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/other_sessions.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/other_sessions.js:248: if (isTabSyncEnabled) if we end up keeping .invisible this.classList.toggle(isTabSyncEnabled, 'invisible'); https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/recently_closed.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/recently_closed.js:66: if (dataItems.length) same nit re: this.classList.toggle()
I've responded to some of your comments, and I'm working on the others. https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.css (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:185: -webkit-justify-content: space-between; On 2013/01/31 20:22:11, Dan Beam wrote: > why is -webkit-justify-content needed? Re: justify, .invisible, etc: I think explaining the overall approach should help, or at least perhaps you can suggest a better approach since I'm far from fluent in CSS. First, the goal is to keep the nav dots centered in the footer regardless of how many of the right-side buttons are visible. Otherwise, when they're all present or all missing, the nav dots are significantly off-centered and look pretty funny. The footer contents are then split into three groups: the logo (left side), the nav dots, and the buttons (right side). Storing these three containers in a flexbox with "justify-content: space-between" ensures that the logo sticks to the left side of the footer, the button group to the right side of the footer, and the empty spaces between the logo and nav dots and the nav dots and button group are equal widths. [ logo ] ----(space)---- [ nav dots ] ----(space)---- [ button group ] https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:458: #recently-closed-menu-button.invisible { On 2013/01/31 20:22:11, Dan Beam wrote: > why did you bring back this whole .invisible business? IIRC it messes up the > centering of the nav "dots" ([ Most visited ][ Apps ] thingies)? The problem is that the button group has variable width when any of the buttons are "hidden: true", so as the button group expands/contracts, the nav dots will shift. So 'visibility: hidden' keeps the button group a fixed size. (Patrick thought the .invisible approach was the right way, and we couldn't really figure out from the Git logs why it was removed. You're saying it broke the centering?) https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.html:118: <a id="chrome-web-store-link"> On 2013/01/31 20:22:11, Dan Beam wrote: > this is not a menu so it doesn't belong here I was reusing the container to keep all the buttons in one element (see previous comment). Can I perhaps rename the container? https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.js:345: logo.style.width = menu.clientWidth + 'px'; On 2013/01/31 20:22:11, Dan Beam wrote: > this shouldn't be necessary and can be done in CSS Continuing from the previous comment, even with the button group a fixed size, the logo and button group aren't necessarily equal widths, so the nav dots can still be off-centered. Patrick suggested detecting the button group width on page load and padding the logo. I can't find anything in CSS that allows you to express that one element should have the same width as another--can you point me in the right direction?
https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/new_tab.css (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:184: -webkit-flex-direction: row; On 2013/01/31 20:22:11, Dan Beam wrote: > this seems to be the default value (so probably not needed) Done. https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:437: .footer-menu-button:not(.invisible) ~ #chrome-web-store-link > #vertical-separator { On 2013/01/31 20:22:11, Dan Beam wrote: > 80 col wrap, > isn't needed Done. https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:467: #other-sessions-menu-button.invisible { On 2013/01/31 20:22:11, Dan Beam wrote: > combine with selector above, i.e. > > #chrome-web-store-link.invisible, > #recently-closed-menu-button.invisible, > #other-sessions-menu-button.invisible { > -webkit-order: -1; > visibility: hidden; > } > > or better yet > > #footer-menu-container .invisible { > -webkit-order: -1; > visibility: hidden; > } Done. https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/new_tab.css:472: #other-sessions-menu-button::not(.invisible) { On 2013/01/31 20:22:11, Dan Beam wrote: > ::not() should be :not() (:: means psuedo-element, : means psuedo-selector) Done. https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/other_sessions.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/other_sessions.js:248: if (isTabSyncEnabled) This doesn't work. It coerces the first argument to a string and toggles it, which in this case just adds "true" or "false" to the class list. On 2013/01/31 20:22:11, Dan Beam wrote: > if we end up keeping .invisible > > this.classList.toggle(isTabSyncEnabled, 'invisible'); https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/recently_closed.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/recently_closed.js:66: if (dataItems.length) On 2013/01/31 20:22:11, Dan Beam wrote: > same nit re: this.classList.toggle() Same comment as other_sessions.js--that form of toggle doesn't work. If you don't like the if/else, I could do the following, but it seems a bit obtuse: (dataItems.length ? this.classList.remove : this.classList.add).call(this.classList, 'invisible')
https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp4/other_sessions.js (right): https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp4/other_sessions.js:248: if (isTabSyncEnabled) On 2013/02/04 16:40:45, dconnelly wrote: > This doesn't work. It coerces the first argument to a string and toggles it, > which in this case just adds "true" or "false" to the class list. > > On 2013/01/31 20:22:11, Dan Beam wrote: > > if we end up keeping .invisible > > > > this.classList.toggle(isTabSyncEnabled, 'invisible'); > sorry, meant: this.classList.toggle('invisible', isTabSyncEnabled);
On 2013/02/04 19:01:10, Dan Beam wrote: > https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... > File chrome/browser/resources/ntp4/other_sessions.js (right): > > https://chromiumcodereview.appspot.com/12038090/diff/1/chrome/browser/resourc... > chrome/browser/resources/ntp4/other_sessions.js:248: if (isTabSyncEnabled) > On 2013/02/04 16:40:45, dconnelly wrote: > > This doesn't work. It coerces the first argument to a string and toggles it, > > which in this case just adds "true" or "false" to the class list. > > > > On 2013/01/31 20:22:11, Dan Beam wrote: > > > if we end up keeping .invisible > > > > > > this.classList.toggle(isTabSyncEnabled, 'invisible'); > > > > sorry, meant: > this.classList.toggle('invisible', isTabSyncEnabled); Done
wouldn't laying stuff out like this (2 evenly flexed nodes on both sides) make sure that the space on both sides of the "nav dots" is even (and they stay centered)? http://jsfiddle.net/A6VVt/2/
this would avoid the need for JS
On 2013/02/05 20:10:46, Dan Beam wrote: > wouldn't laying stuff out like this (2 evenly flexed nodes on both sides) make > sure that the space on both sides of the "nav dots" is even (and they stay > centered)? http://jsfiddle.net/A6VVt/2/ That doesn't center the dots in the window, it centers them between the logo and whatever's on the right. That is the current behaviour, but it means that the dots move when e.g. Recently Closed becomes visible. Evan asked Daniel to fix that, which is what this CL is about.
considering estade@ is an OWNER, there's no bug ######, and he's the one that knows the problem and asked for it to begin with, he should probably be the one reviewing
looks pretty much ok to me https://chromiumcodereview.appspot.com/12038090/diff/20001/chrome/browser/res... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/12038090/diff/20001/chrome/browser/res... chrome/browser/resources/ntp4/new_tab.html:119: <div id="vertical-separator"></div> seems that clicking on the separator shouldn't navigate to the webstore
On 2013/02/05 22:19:03, Dan Beam wrote: > considering estade@ is an OWNER, there's no bug ######, and he's the one that > knows the problem and asked for it to begin with, he should probably be the one > reviewing Sorry Dan, I pinged you because Evan was OOO and his email said return date unsure. Thanks for your help!
https://chromiumcodereview.appspot.com/12038090/diff/20001/chrome/browser/res... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/12038090/diff/20001/chrome/browser/res... chrome/browser/resources/ntp4/new_tab.html:119: <div id="vertical-separator"></div> On 2013/02/05 23:26:47, Evan Stade wrote: > seems that clicking on the separator shouldn't navigate to the webstore Done.
https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/res... File chrome/browser/resources/ntp4/new_tab.css (right): https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/res... chrome/browser/resources/ntp4/new_tab.css:439: -webkit-order: 2; I think the order should always remain the same, or else the webstore link will jump around when a menu becomes visible, no?
https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/res... File chrome/browser/resources/ntp4/new_tab.css (right): https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/res... chrome/browser/resources/ntp4/new_tab.css:439: -webkit-order: 2; On 2013/02/06 22:09:14, Evan Stade wrote: > I think the order should always remain the same, or else the webstore link will > jump around when a menu becomes visible, no? I don't understand--this just selects the separator. The order of non-invisible elements is fixed: 0: other sessions, 1: recently closed, 2: separator, 3: webstore
https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/res... File chrome/browser/resources/ntp4/new_tab.css (right): https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/res... chrome/browser/resources/ntp4/new_tab.css:439: -webkit-order: 2; On 2013/02/06 23:09:24, dconnelly wrote: > On 2013/02/06 22:09:14, Evan Stade wrote: > > I think the order should always remain the same, or else the webstore link > will > > jump around when a menu becomes visible, no? > > I don't understand--this just selects the separator. The order of non-invisible > elements is fixed: 0: other sessions, 1: recently closed, 2: separator, 3: > webstore my question is why it moves when it is invisible.
It only moves to the left of the container (order: -1), just like the other elements do when they're invisible. I don't know what you mean about the webstore link jumping--the separator is ordered between the webstore link and the buttons, so when a menu button becomes visible, the separator shows up to the left of the webstore link and the menu button to the left of that. The webstore link doesn't jump, it just stays glued to the right side. On Thursday, February 7, 2013, wrote: > > https://chromiumcodereview.**appspot.com/12038090/diff/** > 28001/chrome/browser/**resources/ntp4/new_tab.css<https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/resources/ntp4/new_tab.css> > File chrome/browser/resources/ntp4/**new_tab.css (right): > > https://chromiumcodereview.**appspot.com/12038090/diff/** > 28001/chrome/browser/**resources/ntp4/new_tab.css#**newcode439<https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/resources/ntp4/new_tab.css#newcode439> > chrome/browser/resources/ntp4/**new_tab.css:439: -webkit-order: 2; > On 2013/02/06 23:09:24, dconnelly wrote: > >> On 2013/02/06 22:09:14, Evan Stade wrote: >> > I think the order should always remain the same, or else the >> > webstore link > >> will >> > jump around when a menu becomes visible, no? >> > > I don't understand--this just selects the separator. The order of >> > non-invisible > >> elements is fixed: 0: other sessions, 1: recently closed, 2: >> > separator, 3: > >> webstore >> > > my question is why it moves when it is invisible. > > https://chromiumcodereview.**appspot.com/12038090/<https://chromiumcodereview... >
On 2013/02/07 08:08:30, dconnelly_google.com wrote: > It only moves to the left of the container (order: -1), just like the other > elements do when they're invisible. I don't know what you mean about the > webstore link jumping--the separator is ordered between the webstore link > and the buttons, so when a menu button becomes visible, the separator shows > up to the left of the webstore link and the menu button to the left of > that. The webstore link doesn't jump, it just stays glued to the right > side. > > On Thursday, February 7, 2013, wrote: > > > > > https://chromiumcodereview.**appspot.com/12038090/diff/** > > > 28001/chrome/browser/**resources/ntp4/new_tab.css<https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/resources/ntp4/new_tab.css> > > File chrome/browser/resources/ntp4/**new_tab.css (right): > > > > https://chromiumcodereview.**appspot.com/12038090/diff/** > > > 28001/chrome/browser/**resources/ntp4/new_tab.css#**newcode439<https://chromiumcodereview.appspot.com/12038090/diff/28001/chrome/browser/resources/ntp4/new_tab.css#newcode439> > > chrome/browser/resources/ntp4/**new_tab.css:439: -webkit-order: 2; > > On 2013/02/06 23:09:24, dconnelly wrote: > > > >> On 2013/02/06 22:09:14, Evan Stade wrote: > >> > I think the order should always remain the same, or else the > >> > > webstore link > > > >> will > >> > jump around when a menu becomes visible, no? > >> > > > > I don't understand--this just selects the separator. The order of > >> > > non-invisible > > > >> elements is fixed: 0: other sessions, 1: recently closed, 2: > >> > > separator, 3: > > > >> webstore > >> > > > > my question is why it moves when it is invisible. > > > > > https://chromiumcodereview.**appspot.com/12038090/%3Chttps://chromiumcoderevi...> > > Ping
sorry for delay. lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038090/20008
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038090/20008
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038090/20008
Message was sent while issue was closed.
Change committed as 182190 |