Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1585473003:
MD History: Added a clear browsing data button (Closed)
DescriptionMD History: Added a clear browsing data button.
This CL adds a clear browsing data button that navigates the
user to the settings page.
BUG=425625
Committed: https://crrev.com/76ffcc0ff3cff928046a5235bf076acd416debc9
Cr-Commit-Position: refs/heads/master@{#374805}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : the formatting thing #
Total comments: 6
Patch Set 4 : Address reviwer's comments #
Total comments: 4
Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : comments #
Total comments: 3
Patch Set 7 : address comments #
Total comments: 4
Patch Set 8 : Renames + clean up css #
Total comments: 2
Patch Set 9 : ye #
Messages
Total messages: 44 (15 generated)
yingran@google.com changed reviewers: + calamity@chromium.org, tsergeant@chromium.org
Description was changed from ========== Added a clear browsing data button that navigates the user to the settings page. BUG=425652 ========== to ========== Added a clear browsing data button that navigates the user to the settings page. BUG=425625 ==========
Patchset #2 (id:20001) has been deleted
Not 100% sure how to do this, since it's completely unspecced. At the least, can you make it match the style of MD Downloads, so that it aligns with the left side of the cards?
On 2016/02/02 03:59:43, tsergeant wrote: > Not 100% sure how to do this, since it's completely unspecced. At the least, can > you make it match the style of MD Downloads, so that it aligns with the left > side of the cards? I followed a WIP preview here: https://folio.googleplex.com/chrome-ux/mocks/345-webUI/history#%2F03-single.p... I can still change it to align to the left side of the card?
In that case, sure. We can see what Alan thinks once it's on Canary. lgtm w/ a nit. https://codereview.chromium.org/1585473003/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:52: margin-right: 30px; Nit: RTL
Oh, and one more nit: Please update your CL description (at least, so that it matches the title of the CL)
Description was changed from ========== Added a clear browsing data button that navigates the user to the settings page. BUG=425625 ========== to ========== MD History: Added a clear browsing data button Added a clear browsing data button that navigates the user to the settings page. BUG=425625 ==========
Description was changed from ========== MD History: Added a clear browsing data button Added a clear browsing data button that navigates the user to the settings page. BUG=425625 ========== to ========== MD History: Added a clear browsing data button The clear browsing data button that navigates the user to the settings page. BUG=425625 ==========
lgtm.
Description was changed from ========== MD History: Added a clear browsing data button The clear browsing data button that navigates the user to the settings page. BUG=425625 ========== to ========== MD History: Added a clear browsing data button. This CL adds a clear browsing data button that navigates the user to the settings page. BUG=425625 ==========
On 2016/02/03 00:54:31, calamity wrote: > lgtm. so, uh, you should probably switch this to above the MD Downloads style, as discussed in the meeting.
https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:61: @apply(--layout-center-justified); Is this one necessary? https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:78: min-width: 80px; I would prefer it if this was a bit bigger -- in the 90-100px range. https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:108: <div id="button-container"> This is all still visible when there are items selected.
https://codereview.chromium.org/1585473003/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:52: margin-right: 30px; On 2016/02/02 04:21:33, tsergeant wrote: > Nit: RTL Acknowledged. https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:61: @apply(--layout-center-justified); On 2016/02/03 22:57:11, tsergeant wrote: > Is this one necessary? Done. https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:78: min-width: 80px; On 2016/02/03 22:57:11, tsergeant wrote: > I would prefer it if this was a bit bigger -- in the 90-100px range. Done. https://codereview.chromium.org/1585473003/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:108: <div id="button-container"> On 2016/02/03 22:57:11, tsergeant wrote: > This is all still visible when there are items selected. Done.
Generally looks good, but I'm deferring this review to Chris, since he's the one who had the idea for this, uh, 'solution'. https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:114: <paper-button on-tap="clearBrowsingData" i18n-content="clearBrowsingData"> Rename this event to the standard format: onClearBrowsingDataTap_ (Also, wrap to 80 characters)
Hannah just landed a change to the toolbar, so you're going to need to rebase this as well.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:114: <paper-button on-tap="clearBrowsingData" i18n-content="clearBrowsingData"> On 2016/02/04 02:49:02, tsergeant wrote: > Rename this event to the standard format: > > onClearBrowsingDataTap_ > > (Also, wrap to 80 characters) Done.
https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:115: <div id="clear-browsing-data"> Use hidden$= here instead of hiding with CSS, since this is how the rest of the toolbar does it now.
https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:53: flex: 1 1 960px; We should convert this 960px value into a variable in a shared style sheet that we can use across elements. Or a mixin. Either way, I don't want to have to change this value in 5 different places when UX wants the card thinner. (Follow-up patch) https://codereview.chromium.org/1585473003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:93: i18n-values="label:search;clear-label:clearSearch"></cr-search-field> nit: Wrap. https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:57: pointer-events: auto; This pointer-events style needs to be on the button, not on the div containing the button. https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:88: pointer-events: none; Remove.
https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:57: pointer-events: auto; On 2016/02/05 00:43:37, calamity wrote: > This pointer-events style needs to be on the button, not on the div containing > the button. Done. https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:88: pointer-events: none; On 2016/02/05 00:43:38, calamity wrote: > Remove. Done. https://codereview.chromium.org/1585473003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:115: <div id="clear-browsing-data"> On 2016/02/05 00:16:03, tsergeant wrote: > Use hidden$= here instead of hiding with CSS, since this is how the rest of the > toolbar does it now. Done.
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585473003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585473003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1585473003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:32: @apply(--layout-horizontal); Remove. https://codereview.chromium.org/1585473003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:61: } I think paper-button { pointer-events: auto; } might be nicer.
https://codereview.chromium.org/1585473003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:61: } On 2016/02/08 00:11:37, calamity wrote: > I think > > paper-button { > pointer-events: auto; > } > > might be nicer. Done.
🚂 almost there 🚂 https://codereview.chromium.org/1585473003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:20: #main-content { Add #button-container and #toolbar-container to this and remove the identical lines below. https://codereview.chromium.org/1585473003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:113: <div id="clear-browsing-data" hidden$="{{itemsSelected_}}"> Maybe rename this div to something more generic, in case we want to add more buttons here in the future? How about #centered-buttons ?
https://codereview.chromium.org/1585473003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:20: #main-content { On 2016/02/09 03:17:52, tsergeant wrote: > Add #button-container and #toolbar-container to this and remove the identical > lines below. Done. https://codereview.chromium.org/1585473003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:113: <div id="clear-browsing-data" hidden$="{{itemsSelected_}}"> On 2016/02/09 03:17:52, tsergeant wrote: > Maybe rename this div to something more generic, in case we want to add more > buttons here in the future? > > How about > > #centered-buttons ? Done.
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585473003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585473003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm again
lgtm w/ a nit. https://codereview.chromium.org/1585473003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:90: i18n-values="label:search;clear-label:clearSearch"></cr-search-field> nit: Wrap the close tag.
https://codereview.chromium.org/1585473003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1585473003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:90: i18n-values="label:search;clear-label:clearSearch"></cr-search-field> On 2016/02/09 11:17:39, calamity wrote: > nit: Wrap the close tag. Done.
The CQ bit was checked by yingran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/1585473003/#ps200001 (title: "ye")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585473003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585473003/200001
Message was sent while issue was closed.
Description was changed from ========== MD History: Added a clear browsing data button. This CL adds a clear browsing data button that navigates the user to the settings page. BUG=425625 ========== to ========== MD History: Added a clear browsing data button. This CL adds a clear browsing data button that navigates the user to the settings page. BUG=425625 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Added a clear browsing data button. This CL adds a clear browsing data button that navigates the user to the settings page. BUG=425625 ========== to ========== MD History: Added a clear browsing data button. This CL adds a clear browsing data button that navigates the user to the settings page. BUG=425625 Committed: https://crrev.com/76ffcc0ff3cff928046a5235bf076acd416debc9 Cr-Commit-Position: refs/heads/master@{#374805} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/76ffcc0ff3cff928046a5235bf076acd416debc9 Cr-Commit-Position: refs/heads/master@{#374805} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
