Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2316423003:
NTP: fade-in the tiles only when they change, not when they are shown initially (Closed)
|
Created:
4 years, 3 months ago by Marc Treib Modified:
4 years, 3 months ago Reviewers:
jkrcal CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP: fade-in the tiles only when they change, not when they are shown initially
Before this CL, the fade-in transition would trigger inconsistently, maybe in 1 out of 10 cases. Now it triggers consistently, only when the tiles change, but not when the first set of tiles is shown.
BUG=645158
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7c098a4936916cfb82e88669e39a5be4f11b46ac
Cr-Commit-Position: refs/heads/master@{#417539}
Patch Set 1 #Patch Set 2 : . #
Total comments: 8
Messages
Total messages: 17 (10 generated)
Description was changed from ========== NTP: fade-in the tiles only when they change, not when they are shown initially BUG=645158 ========== to ========== NTP: fade-in the tiles only when they change, not when they are shown initially BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== NTP: fade-in the tiles only when they change, not when they are shown initially BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP: fade-in the tiles only when they change, not when they are shown initially Before this CL, the fade-in transition would trigger inconsistently, maybe in 1 out of 10 cases. Now it triggers consistently, only when the tiles change, but not when the first set of tiles is shown. BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
treib@chromium.org changed reviewers: + jkrcal@chromium.org
PTAL! https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (left): https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:279: setTimeout(function() { This was the root problem: When the timeout function runs, the original style may or may not have been applied, hence the inconsistent behavior. https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:283: window.getComputedStyle(cur).opacity; From https://timtaubert.de/blog/2012/09/css-transitions-for-dynamically-created-do... :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:264: if (old) { I am puzzled: How come this is not true initially when you added the div#mv-tiles into the html? https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:283: window.getComputedStyle(cur).opacity; On 2016/09/08 18:17:48, Marc Treib wrote: > From > https://timtaubert.de/blog/2012/09/css-transitions-for-dynamically-created-do... > :) This looks hacky :) I haven't found any cleaner solution, though. So, okay...
https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:264: if (old) { On 2016/09/09 08:13:13, jkrcal wrote: > I am puzzled: > How come this is not true initially when you added the div#mv-tiles into the > html? When this runs for the first time, there is no div#mv-tiles (it used to be in the html, but I removed it, since there's no reason to have it there initially as far as I can tell). https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:283: window.getComputedStyle(cur).opacity; On 2016/09/09 08:13:13, jkrcal wrote: > On 2016/09/08 18:17:48, Marc Treib wrote: > > From > > > https://timtaubert.de/blog/2012/09/css-transitions-for-dynamically-created-do... > > :) > > This looks hacky :) I haven't found any cleaner solution, though. So, okay... It's not pretty, but I'd argue it's less hacky than the previous setTimeout hack. And at least, it seems to work consistently!
LGTM, thanks! https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:264: if (old) { On 2016/09/09 08:56:28, Marc Treib wrote: > On 2016/09/09 08:13:13, jkrcal wrote: > > I am puzzled: > > How come this is not true initially when you added the div#mv-tiles into the > > html? > > When this runs for the first time, there is no div#mv-tiles (it used to be in > the html, but I removed it, since there's no reason to have it there initially > as far as I can tell). Huh, I've swapped the direction :) Sure, this makes sense, thanks! https://codereview.chromium.org/2316423003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:283: window.getComputedStyle(cur).opacity; On 2016/09/09 08:56:28, Marc Treib wrote: > On 2016/09/09 08:13:13, jkrcal wrote: > > On 2016/09/08 18:17:48, Marc Treib wrote: > > > From > > > > > > https://timtaubert.de/blog/2012/09/css-transitions-for-dynamically-created-do... > > > :) > > > > This looks hacky :) I haven't found any cleaner solution, though. So, okay... > > It's not pretty, but I'd argue it's less hacky than the previous setTimeout > hack. And at least, it seems to work consistently! That's true, probably less hacky.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== NTP: fade-in the tiles only when they change, not when they are shown initially Before this CL, the fade-in transition would trigger inconsistently, maybe in 1 out of 10 cases. Now it triggers consistently, only when the tiles change, but not when the first set of tiles is shown. BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP: fade-in the tiles only when they change, not when they are shown initially Before this CL, the fade-in transition would trigger inconsistently, maybe in 1 out of 10 cases. Now it triggers consistently, only when the tiles change, but not when the first set of tiles is shown. BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== NTP: fade-in the tiles only when they change, not when they are shown initially Before this CL, the fade-in transition would trigger inconsistently, maybe in 1 out of 10 cases. Now it triggers consistently, only when the tiles change, but not when the first set of tiles is shown. BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP: fade-in the tiles only when they change, not when they are shown initially Before this CL, the fade-in transition would trigger inconsistently, maybe in 1 out of 10 cases. Now it triggers consistently, only when the tiles change, but not when the first set of tiles is shown. BUG=645158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7c098a4936916cfb82e88669e39a5be4f11b46ac Cr-Commit-Position: refs/heads/master@{#417539} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7c098a4936916cfb82e88669e39a5be4f11b46ac Cr-Commit-Position: refs/heads/master@{#417539} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
