Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864023002:
MD History: Add spinners when new data is loading or searching (Closed)
|
Created:
4 years, 8 months ago by lshang Modified:
4 years, 8 months ago Reviewers:
tsergeant CC:
chromium-reviews, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: Add spinners when new data is loading or searching
Add a spinner indicating that more history is loading when scroll down to the
bottom of the page.
Also add a spinner when searching terms in history.
BUG=595588
Committed: https://crrev.com/3f88d690f487221c8a78d54e84c502155de80366
Cr-Commit-Position: refs/heads/master@{#389645}
Patch Set 1 #
Total comments: 8
Patch Set 2 : move searching spinner to toolbar #Patch Set 3 : wait for upgrade toolbar #
Total comments: 11
Patch Set 4 : address review comments #Patch Set 5 : import paper-spinner in toolbar #
Total comments: 8
Patch Set 6 : #Patch Set 7 : rebase #Patch Set 8 : adjust search spinner after rebase #
Total comments: 2
Patch Set 9 : use id for paper-spinner #Patch Set 10 : change loading spinner #
Messages
Total messages: 22 (7 generated)
Description was changed from ========== MD History: Add spinners when new data is loading or searching format update searching spinner position update search spinner add spinner BUG= ========== to ========== MD History: Add spinners when new data is loading or searching Add a spinner indicating that more history is loading when scroll down to the bottom of the page. Also add a spinner when searching terms in history. BUG=595588 ==========
lshang@chromium.org changed reviewers: + tsergeant@chromium.org
Hi Tim, this is the change for adding spinners. I'm quite not sure about the <style> part, so looking forward to your comments. :-)
https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.html:24: #loading_spinner { We generally use '#loading-spinner' rather than '#loading_spinner' for element IDs (although in some places we do use '#loadingSpinner' instead). https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.html:26: left: 50%; This will be ~not quite~ centered. This is because the spinner will start 50% from the left, then the spinner will be rendered, then there'll only be something like 45% of space left right of the center. To fix this, you need to make sure that the size of the spinner itself is included in the calculation. You can do this with: position: absolute left: 0 right: 0 margin: 0 auto; width: 28px; (shorthand for margin-left: auto, margin-right: auto) Centering something in CSS is one of the hardest problems in software engineering. https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.html:73: <div id="searching_spinner" hidden$="[[!searching_]]"> It would be nice if this was part of the <history-toolbar> rather than <history-list>, since that's where it's being positioned. https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history_list.js (right): https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.js:60: setSearching: function() { We need to be careful about changing when loading_ is true, since it also: * Disables loading more data by scrolling down * Controls what messages can appear over the page. If you want to have a separate searching_ boolean, you'll still need to make sure that loading_ is true.
Thanks for your patient explanation Tim! PTAL thanks! I change to waitForUpgrade toolbar, because I found that if check for info.term, when searching a term and clear it, the searching spinner will show up since search-changed and won't hide any more, until search for something else. https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.html:24: #loading_spinner { On 2016/04/07 01:07:02, tsergeant wrote: > We generally use '#loading-spinner' rather than '#loading_spinner' for element > IDs (although in some places we do use '#loadingSpinner' instead). Done. https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.html:26: left: 50%; On 2016/04/07 01:07:02, tsergeant wrote: > This will be ~not quite~ centered. This is because the spinner will start 50% > from the left, then the spinner will be rendered, then there'll only be > something like 45% of space left right of the center. > > To fix this, you need to make sure that the size of the spinner itself is > included in the calculation. You can do this with: > > position: absolute > left: 0 > right: 0 > margin: 0 auto; > width: 28px; > (shorthand for margin-left: auto, margin-right: auto) > > Centering something in CSS is one of the hardest problems in software > engineering. Done. And I think it might look better if the spinner is at the center of history-list rather than the whole page, so I add a 200px left, which is the width of history side bar. https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.html:73: <div id="searching_spinner" hidden$="[[!searching_]]"> On 2016/04/07 01:07:02, tsergeant wrote: > It would be nice if this was part of the <history-toolbar> rather than > <history-list>, since that's where it's being positioned. Done. https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history_list.js (right): https://codereview.chromium.org/1864023002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history_list.js:60: setSearching: function() { On 2016/04/07 01:07:02, tsergeant wrote: > We need to be careful about changing when loading_ is true, since it also: > > * Disables loading more data by scrolling down > * Controls what messages can appear over the page. > > If you want to have a separate searching_ boolean, you'll still need to make > sure that loading_ is true. Done. Thanks for reminding!
https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:26: left: 200px; This 200px is good when the sidebar exists, but when the user is signed out and there is no sidebar, then the spinner will be off-center again. One way to solve this is to set `position: relative` on the <history-list> (in the :host block above). Then, the #loading-spinner gets positioned relative to the <history-list> instead of relative to the whole page. https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:62: <paper-spinner alt="Loading more history" active></paper-spinner> The "Loading more history" string needs to be localized. I think it would be fine to reuse the "Loading..." string from history.html, which can be accessed with `$i18n{loading}` https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:45: top: 15px; Is this necessary? https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:98: <paper-spinner class="search" alt="searching in history" active> Same comment here about localization. However, since the alt-text is only used by screen readers, depending on how the screen reader works, it might not be necessary to set the alt-text on both spinners -- it might work fine if you leave one empty. https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.js:78: * Mark the toolbar as currently searching term from the back-end. You can either replace these two functions with a single one: setSearching(value) Or make the `searching_` property public and have history.js modify it directly: $('toolbar').searching = true; ^ This second one has the advantage of probably not needing waitForUpgrade anymore.
Thanks Tim! Can you PTAL again? https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:26: left: 200px; On 2016/04/15 05:14:50, tsergeant wrote: > This 200px is good when the sidebar exists, but when the user is signed out and > there is no sidebar, then the spinner will be off-center again. > > One way to solve this is to set `position: relative` on the <history-list> (in > the :host block above). > > Then, the #loading-spinner gets positioned relative to the <history-list> > instead of relative to the whole page. Done. An absolute position element is positioned relative to the first parent element that has a position other than static. OMG CSS is so mysterious! https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:62: <paper-spinner alt="Loading more history" active></paper-spinner> On 2016/04/15 05:14:50, tsergeant wrote: > The "Loading more history" string needs to be localized. > > I think it would be fine to reuse the "Loading..." string from history.html, > which can be accessed with `$i18n{loading}` Done. https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:45: top: 15px; On 2016/04/15 05:14:50, tsergeant wrote: > Is this necessary? Done. https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:98: <paper-spinner class="search" alt="searching in history" active> On 2016/04/15 05:14:50, tsergeant wrote: > Same comment here about localization. > > However, since the alt-text is only used by screen readers, depending on how the > screen reader works, it might not be necessary to set the alt-text on both > spinners -- it might work fine if you leave one empty. Done. https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.js:78: * Mark the toolbar as currently searching term from the back-end. On 2016/04/15 05:14:50, tsergeant wrote: > You can either replace these two functions with a single one: > setSearching(value) > > Or make the `searching_` property public and have history.js modify it directly: > > $('toolbar').searching = true; > > ^ This second one has the advantage of probably not needing waitForUpgrade > anymore. Done.
Looking good, just a few nits. https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:26: left: 200px; On 2016/04/18 00:29:00, Liu Shang wrote: > On 2016/04/15 05:14:50, tsergeant wrote: > > This 200px is good when the sidebar exists, but when the user is signed out > and > > there is no sidebar, then the spinner will be off-center again. > > > > One way to solve this is to set `position: relative` on the <history-list> (in > > the :host block above). > > > > Then, the #loading-spinner gets positioned relative to the <history-list> > > instead of relative to the whole page. > > Done. > An absolute position element is positioned relative to the first parent element > that has a position other than static. OMG CSS is so mysterious! Yup, it's pretty weird sometimes. https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:94: $('toolbar').searching_ = false; Nit: Add the /** @type {HistoryToolbarElement} */ annotation, like above on line 69. You can run the closure compiler which uses these annotations with: third_party/closure_compiler/run_compiler (there are full docs here: https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...) https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:64: <paper-spinner alt=$i18n{loading} active></paper-spinner> Nit: This should have quotes alt="$i18n{loading}" https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:94: <div id="searching-spinner" hidden$="[[!searching_]]"> Nit: There's an extra space in here. https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.js:47: searching_: { Nit: Rename this to `searching` with no underscore to show that it's a public property. (In javascript, we use an underscore at the end of a name to mark a method or property as private).
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Thanks Tim! Can you take a look again? https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:94: $('toolbar').searching_ = false; On 2016/04/18 01:26:36, tsergeant wrote: > Nit: Add the /** @type {HistoryToolbarElement} */ annotation, like above on line > 69. > > You can run the closure compiler which uses these annotations with: > third_party/closure_compiler/run_compiler > > (there are full docs here: > https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...) Done. https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_list.html (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_list.html:64: <paper-spinner alt=$i18n{loading} active></paper-spinner> On 2016/04/18 01:26:36, tsergeant wrote: > Nit: This should have quotes > > alt="$i18n{loading}" Done. https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.html:94: <div id="searching-spinner" hidden$="[[!searching_]]"> On 2016/04/18 01:26:36, tsergeant wrote: > Nit: There's an extra space in here. Done. https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/1864023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.js:47: searching_: { On 2016/04/18 01:26:36, tsergeant wrote: > Nit: Rename this to `searching` with no underscore to show that it's a public > property. > > (In javascript, we use an underscore at the end of a name to mark a method or > property as private). Done.
And closure compile pass:-)
Just one more nit https://codereview.chromium.org/1864023002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1864023002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:110: <paper-spinner class="search" active> Nit: Instead of class="search", you can move id="searching-spinner" down to this paper-spinner and use that in the CSS above. FYI, we typically only use classes for things where you want the same styling to apply to multiple different elements. IDs are more useful for applying styles to a single element.
https://codereview.chromium.org/1864023002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/1864023002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:110: <paper-spinner class="search" active> On 2016/04/22 02:18:44, tsergeant wrote: > Nit: Instead of class="search", you can move id="searching-spinner" down to this > paper-spinner and use that in the CSS above. > Done. After I removed the class and moved id "searching-spinner" into paper-spinner, I feel like there is no benefit to still have a div here? (And the div has no ids, just a hidden property.) So I use 'searching' to directly control the 'active' property of paper-spinner. I'm not fully sure this is appropriate, but if you feel ok with this, I might change the loading spinner as well, because I found that it looks more smooth when the spinner switches between active/not-active mode. It has a fade-in/fade-out effect. > FYI, we typically only use classes for things where you want the same styling to > apply to multiple different elements. IDs are more useful for applying styles to > a single element. Got it!:-)
Also changed the loading spinner. Confirm the changes please~
lgtm
The CQ bit was checked by lshang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864023002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864023002/220001
Message was sent while issue was closed.
Description was changed from ========== MD History: Add spinners when new data is loading or searching Add a spinner indicating that more history is loading when scroll down to the bottom of the page. Also add a spinner when searching terms in history. BUG=595588 ========== to ========== MD History: Add spinners when new data is loading or searching Add a spinner indicating that more history is loading when scroll down to the bottom of the page. Also add a spinner when searching terms in history. BUG=595588 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Add spinners when new data is loading or searching Add a spinner indicating that more history is loading when scroll down to the bottom of the page. Also add a spinner when searching terms in history. BUG=595588 ========== to ========== MD History: Add spinners when new data is loading or searching Add a spinner indicating that more history is loading when scroll down to the bottom of the page. Also add a spinner when searching terms in history. BUG=595588 Committed: https://crrev.com/3f88d690f487221c8a78d54e84c502155de80366 Cr-Commit-Position: refs/heads/master@{#389645} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3f88d690f487221c8a78d54e84c502155de80366 Cr-Commit-Position: refs/heads/master@{#389645} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
