Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088093004:
Add a footer about other forms of history to the MD history page. (Closed)
|
Created:
4 years, 6 months ago by msramek Modified:
4 years, 5 months ago CC:
chromium-reviews, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a footer about other forms of history to the MD history page.
This is a followup to https://codereview.chromium.org/1838333004/ where
it was added to the history WebUI.
Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20History/MD%20Update
What's not seen on Folio, but was discussed offline: Although it's
positioned in the side bar, the footer should not behave like
a paper-item. Only the link itself should be clickable.
Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is
a new screenshot after the side-bar layout changed (used to be static,
after https://codereview.chromium.org/2020963002/ it's hidden
under the hamburger menu.
BUG=595332
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/59db76ef6f2b5d45b17aeb1f6cc48c083178df10
Cr-Commit-Position: refs/heads/master@{#406055}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Initialize to false. #
Total comments: 6
Patch Set 3 : Rebased. #Patch Set 4 : Addressed comments. #
Total comments: 12
Patch Set 5 : Addressed comments. #Patch Set 6 : Rebase. #
Total comments: 4
Patch Set 7 : Added getter. #Patch Set 8 : Rebase. #
Messages
Total messages: 40 (15 generated)
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. BUG=595332 ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. BUG=595332 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
msramek@chromium.org changed reviewers: + calamity@chromium.org, dbeam@chromium.org
Hi Dan and Chris, please have a look! Thanks, Martin https://codereview.chromium.org/2088093004/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/side_bar.html:29: -webkit-padding-start: 24px; I think this should be 24px, since: 1. It's in the spec: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag... 2. The "History" header also uses 24px. 3. It's an anomaly ("-webkit-padding-start: 24px" has more hits in code search than "-webkit-padding-start: 25px").
FYI, the sidebar is changing in https://codereview.chromium.org/2020963002/ into a drawer-style menu that won't be visible by default. This message will be much more hidden. +cc tsergeant@ for opinions and advice.
On 2016/06/24 03:32:22, calamity wrote: > FYI, the sidebar is changing in https://codereview.chromium.org/2020963002/ into > a drawer-style menu that won't be visible by default. This message will be much > more hidden. > > +cc tsergeant@ for opinions and advice. Here's a screenshot of what it would look like in the new layout: https://goto.google.com/kakhi I'm happy for it to go there, depending on how you feel about the reduced visibility.
Thanks for the heads-up. I chatted with our UX designer, and we decided to go forward. The reduced visibility is really not ideal, as this footer is supposed to improve transparency. However, it is acceptable for now, because: - There is a precedent from other MD apps to put their footer in the sidebar, even if the sidebar is hidden by default. - The users we target are those that are going to delete their history, so the footer logically belongs to the same area where the "Clear Browsing Data" button is placed. - It's hard to argue that the footer is a more important element than the navigation, so if the navigation can be hidden by default... We'll see if we can come up with a better idea, but since M53 is drawing close, we'd like to go forward with this implementation. Cheers, Martin
Friendly ping :)
who do you want to review what? do you have screenshots? https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:22: showFooter: { type: Boolean, value: false }, { type: Boolean, value: false } ^ ^ remove these spaces https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:58: .set('showFooter', includeOtherFormsOfBrowsingHistory); nit: you can just do .showFooter = includeOtherFormsOfBrowsingHistory; this is mildly better for typechecking https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:49: font-size: 14px; don't use px for fonts, use relative units (i.e. % or em) so that if a user changes their default font size this UI scales with it
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. BUG=595332 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Dan - I'd like an owner's review from you, but I added Chris as he's been working on the history page for additional feedback and coordination. I rebased over https://codereview.chromium.org/2020963002/, which moved the sidebar under the hamburger menu, and uploaded a new screenshot. But this is mostly a detail; the original spec (folio link in the description) still applies. https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:22: showFooter: { type: Boolean, value: false }, On 2016/06/30 23:30:03, Dan Beam wrote: > { type: Boolean, value: false } > ^ ^ > remove these spaces Done. Also below (since this is a copy-paste from there). https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:58: .set('showFooter', includeOtherFormsOfBrowsingHistory); On 2016/06/30 23:30:03, Dan Beam wrote: > nit: you can just do > > .showFooter = includeOtherFormsOfBrowsingHistory; > > this is mildly better for typechecking Done. https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:49: font-size: 14px; On 2016/06/30 23:30:03, Dan Beam wrote: > don't use px for fonts, use relative units (i.e. % or em) so that if a user > changes their default font size this UI scales with it Makes sense. It seems that after the rebase I can actually remove this, since the document structure change and whatever I wanted to override with this is no longer in effect.
"Dan - I'd like an owner's review from you, but I added Chris as he's been working on the history page for additional feedback and coordination." we're both owners, but calamity@'s got a better familiarity with history than me lately. this code looks OK but 2 questions: 1) I don't like that we need to set a bottom of 120px. why does the drawer "protude"? can we fix that instead of setting this value? it should be bottom: 0 afaict 2) should clicking the <a> close the drawer? https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:62: the viewport. */ /* This compensates the 120px by which app-banner protrudes under * the viewport. */ ^
Yeah, that 120px thing is weird. It's in app-drawer. No idea why. https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:48: function showNotification( Does this get called when the user signs in for the first time? https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:58: .showFooter = includeOtherFormsOfBrowsingHistory; Just access the sidebar through (appElem).$['side-bar'].showFooter here. I'd prefer the property set in appElem stay as minimal as possible. https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:61: /* This compensates the 120px by which app-banner protrudes under by which app-"drawer"? https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:101: <div id="footer-text">$i18nRaw{footer}</div> nit: sidebarFooter perhaps? https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.js (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.js:16: value: false I think you can get away with showFooter: Boolean here. We tend to avoid early initialization where possible.
I explained the 120px shift down in the comments. Regarding the second question, whether clicking <a> should close the drawer - it opens a new tab (target="_blank"), so I believe it should not change the state of this tab. Another argument is that clicking in the app drawer outside of the navigation elements doesn't close the drawer either. Unless that has changed since I updated my checkout - I have seen MD apps where it does. https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:48: function showNotification( On 2016/07/05 04:03:13, calamity wrote: > Does this get called when the user signs in for the first time? No - and it probably should. This is called from BrowsingHistoryHandler, which is not a SyncServiceObserver. It's only called when the history results are updated, which is when the entire page is reloaded or when more results are fetched (going to the next page in the old chrome://history, scrolling down in the new MD one). ForeignSessionHandler *is* a SyncServiceObserver though, so we could use that one instead. I would leave that part to a separate CL - it's a problem on a different layer than this CL (and effects the old chrome://history as well). https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:58: .showFooter = includeOtherFormsOfBrowsingHistory; On 2016/07/05 04:03:13, calamity wrote: > Just access the sidebar through (appElem).$['side-bar'].showFooter here. I'd > prefer the property set in appElem stay as minimal as possible. Done. Also removed the property in history-app. https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:61: /* This compensates the 120px by which app-banner protrudes under On 2016/07/05 04:03:13, calamity wrote: > by which app-"drawer"? Done. Force of habit :) https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:62: the viewport. */ On 2016/07/01 15:38:27, Dan Beam wrote: > /* This compensates the 120px by which app-banner protrudes under > * the viewport. */ > ^ app-drawer, sorry for the confusion :) Specifically here: https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... Which means that if I align my footer to the bottom (absolute positioning), it ends up being 120px below the viewport. You can see in app-drawer.html that it compensates by setting #contentContainer padding: 120px 0. But that has no effect on absolute positioning. If we want to avoid the magic number of 120px, I could use "padding-bottom: inherit" to propagate that padding to my footer. But in that case we're heavily dependent on the HTML structure (e.g. adding an additional div without inheriting the padding would break it), so I don't think that's cleaner. https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.html:101: <div id="footer-text">$i18nRaw{footer}</div> On 2016/07/05 04:03:13, calamity wrote: > nit: sidebarFooter perhaps? Done. https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/side_bar.js (right): https://codereview.chromium.org/2088093004/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/side_bar.js:16: value: false On 2016/07/05 04:03:13, calamity wrote: > I think you can get away with showFooter: Boolean here. We tend to avoid early > initialization where possible. Done. Probably yes - the call setting this value will come very soon after the page is initialized. I was mostly trying to avoid flickering where this would be displayed for a short time after the page reload and then set to false. But that is also mitigated by the fact that the drawer cannot be open on page reload.
Patchset #6 (id:100001) has been deleted
lgtm.
lgtm except for local DOM access (calamity@ or tsergeant@ you agree with this?) https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history.js:58: .$['side-bar'].showFooter = includeOtherFormsOfBrowsingHistory; can we not poke into the local DOM of #history-app? i.e. export the side bar as a property or a getter() or something. https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/side_bar.html:62: the viewport. */ I think you overthought what I meant by my last comment. I just meant: you need a * at the beginning of the second line. As in, the full comment should be /* This compensates the 120px by which app-drawer protrudes under * the viewport. */ ^ | | THIS "*" character here but yes, the 120px this is weird, too. I assume it's for like rubberband/touch scrolling on Safari or something.
https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history.js:58: .$['side-bar'].showFooter = includeOtherFormsOfBrowsingHistory; On 2016/07/12 02:05:57, Dan Beam wrote: > can we not poke into the local DOM of #history-app? i.e. export the side bar as > a property or a getter() or something. It was specifically calamity@ who suggested this in the last iteration to avoid defining additional properties on history-app. Defining a getter for the side bar sounds reasonable to me though - calamity@, would you agree with that? https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/side_bar.html (right): https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/side_bar.html:62: the viewport. */ On 2016/07/12 02:05:57, Dan Beam wrote: > I think you overthought what I meant by my last comment. > > I just meant: you need a * at the beginning of the second line. As in, the full > comment should be > > /* This compensates the 120px by which app-drawer protrudes under > * the viewport. */ > ^ > | > | > THIS "*" character here > > but yes, the 120px this is weird, too. I assume it's for like rubberband/touch > scrolling on Safari or something. Ah :) Done. ( Nevertheless, I wanted to explain the 120px about which you asked at the beginning of the previous iteration. )
On 2016/07/12 13:56:36, msramek wrote: > https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/md_history/history.js (right): > > https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... > chrome/browser/resources/md_history/history.js:58: .$['side-bar'].showFooter = > includeOtherFormsOfBrowsingHistory; > On 2016/07/12 02:05:57, Dan Beam wrote: > > can we not poke into the local DOM of #history-app? i.e. export the side bar > as > > a property or a getter() or something. > > It was specifically calamity@ who suggested this in the last iteration to avoid > defining additional properties on history-app. > > Defining a getter for the side bar sounds reasonable to me though - calamity@, > would you agree with that? > Yeah, that's fine with me. Cheers. > https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/md_history/side_bar.html (right): > > https://codereview.chromium.org/2088093004/diff/120001/chrome/browser/resourc... > chrome/browser/resources/md_history/side_bar.html:62: the viewport. */ > On 2016/07/12 02:05:57, Dan Beam wrote: > > I think you overthought what I meant by my last comment. > > > > I just meant: you need a * at the beginning of the second line. As in, the > full > > comment should be > > > > /* This compensates the 120px by which app-drawer protrudes under > > * the viewport. */ > > ^ > > | > > | > > THIS "*" character here > > > > but yes, the 120px this is weird, too. I assume it's for like > rubberband/touch > > scrolling on Safari or something. > > Ah :) Done. > > ( Nevertheless, I wanted to explain the 120px about which you asked at the > beginning of the previous iteration. )
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
Thanks guys! I had to rebase because of a small merge conflict; proceeding to land now.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2088093004/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by msramek@chromium.org
The CQ bit was checked by msramek@chromium.org
The CQ bit was unchecked by msramek@chromium.org
The CQ bit was checked by msramek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the closure_compilation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified, with these tests: ['defaulttests']
There were warnings when CQ was processing your CL: * CQ is not running the closure_compilation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified, with these tests: ['defaulttests']
There were warnings when CQ was processing your CL: * CQ is not running the closure_compilation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified, with these tests: ['defaulttests']
There were warnings when CQ was processing your CL: * CQ is not running the closure_compilation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified, with these tests: ['defaulttests']
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a footer about other forms of history to the MD history page. This is a followup to https://codereview.chromium.org/1838333004/ where it was added to the history WebUI. Folio: https://folio.googleplex.com/chromenarnia/Clear%20Browsing%20Data%20&%20Histo... What's not seen on Folio, but was discussed offline: Although it's positioned in the side bar, the footer should not behave like a paper-item. Only the link itself should be clickable. Update: https://screenshot.googleplex.com/s2yAqZ05yXT - this is a new screenshot after the side-bar layout changed (used to be static, after https://codereview.chromium.org/2020963002/ it's hidden under the hamburger menu. BUG=595332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/59db76ef6f2b5d45b17aeb1f6cc48c083178df10 Cr-Commit-Position: refs/heads/master@{#406055} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/59db76ef6f2b5d45b17aeb1f6cc48c083178df10 Cr-Commit-Position: refs/heads/master@{#406055} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
