Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1754713005:
[Media Router] Animate transition between sink list and filter views. (Closed)
|
Created:
4 years, 9 months ago by btolsch Modified:
4 years, 7 months ago Reviewers:
apacible CC:
chromium-reviews, media-router+watch_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Animate transition between sink list and filter views.
This change adds:
- Animated transition between sink list view and filter view.
BUG=565696
R=apacible@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a0482de9d6932c61bd89f6cae43f215cc599f0f3
Cr-Commit-Position: refs/heads/master@{#391422}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Responding to comments #
Total comments: 24
Patch Set 3 : Fixed nits and closure compilation #Patch Set 4 : Fixed externs typo #
Total comments: 4
Patch Set 5 : Rebase and fixed nits #Patch Set 6 : Rebase to check for blink bug #Patch Set 7 : Cleaned up animation queueing and tests #
Total comments: 9
Patch Set 8 : Nits and comments #Patch Set 9 : Rebase for test fixes #Patch Set 10 : Rebase for apacible's perf change #Messages
Total messages: 24 (9 generated)
Description was changed from ========== [Media Router] Animmate transition between sink list and filter views. This change adds: - Animated transition between sink list view and filter view. BUG=565696 R=apacible@chromium.org ========== to ========== [Media Router] Animate transition between sink list and filter views. This change adds: - Animated transition between sink list view and filter view. BUG=565696 R=apacible@chromium.org ==========
PTAL, thanks!
I tried out the animations locally, looks great! https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:148: bottom: 0; What are the bottom/left/right/top styles used for? I'm not seeing a difference in behavior when these are disabled. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:151: overflow-y: hidden; overflow-y should be scrollable for small windows with long sink lists. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:193: top: 100%; What is top used for? https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:24: * mechanism. It is null if no current animation is running. Per offline discussion: update this since it's only null when no animation has ever run. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:194: forceSinkListHidden_: { Rename this to something more descriptive, e.g. hideSinkListForAnimation_. Otherwise this seems like an override for showing the sink list at first glance. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:223: heightAdjustPlayer_: { Rename this to something more descriptive, e.g. heightAdjustmentPlayer_. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:229: * Whether the search list is currently hidden. This is closely related to Explain how this is closely related. Why do we need both? For search and animations, we're keeping track of a number of properties, such as: - isSearchListHidden_ - isUserSearching_ - forceSinkListHidden_ - MediaRouterView.FILTER ... etc Can we consolidate some of these to make it simpler? For example, instead of having |isUserSearching_|, set |currentView_| as FILTER. That also removes one of the observer calls. Simplifying the properties in general will also make the states less fragile. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:305: player_: { Rename this to something more descriptive. filterTransitionPlayer_ ? https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1115: * @param {number} speed Optional rate of movement in pixels per second. |speed| isn't specified in any of the function calls. Will this be needed in a later patch? If not, remove it. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1279: var fromHeight = list.offsetHeight + search.offsetHeight; nit: rename to initialHeight and finalHeight. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1288: var listEffect = new KeyframeEffect(list, Add comments on what the expected behavior in this GroupEffect is. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1304: var finalizeAnimation = function() { We talked offline on how this is queued up in situations such as when the user is still searching while the animation is live. Add some comments here summarizing this. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1341: var fromHeight = search.offsetHeight + resultsContainer.offsetHeight; initialHeight/finalHeight here as well. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1552: var toHeight = this.computeTotalSearchHeight_(noMatches, results, search, finalHeight here as well https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1819: this.isUserSearching_ = false; Move into if block.
I added some comments and simplifications. PTAL, thanks! https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:148: bottom: 0; On 2016/03/04 03:18:22, apacible wrote: > What are the bottom/left/right/top styles used for? I'm not seeing a difference > in behavior when these are disabled. bottom: Like top, this is largely conceptual because of how much is controlled dynamically. This stops the visibility of the list at the bottom of the dialog. However, without this I will get occasional flashes of the whole search list when it is animating upwards. left/right: Keeps the container at full width, even when it is positioned absolutely. This only affects the no search matches text during animation. If you remove these and filter so that you get no matches, then click the back arrow, you will see the "no matches" text gets left-aligned as it moves down. top: Similar to |#sink-search|, this is often set dynamically or the element is hidden when it's not. However, this represents the default state of the search results list which is to be off the bottom of the dialog. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:151: overflow-y: hidden; On 2016/03/04 03:18:22, apacible wrote: > overflow-y should be scrollable for small windows with long sink lists. It is set to auto by |putSearchAtTop_| because when it is hidden and when it is changing size there shouldn't be a scrollbar. This is also why it is set separately for x and y: y gets overridden but x is always hidden. I added a comment here. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:193: top: 100%; On 2016/03/04 03:18:22, apacible wrote: > What is top used for? This is used to place the search bar at the bottom of the dialog. It is dynamically given a negative margin equal to its height so it is perfectly at the bottom by default. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:24: * mechanism. It is null if no current animation is running. On 2016/03/04 03:18:23, apacible wrote: > Per offline discussion: update this since it's only null when no animation has > ever run. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:194: forceSinkListHidden_: { On 2016/03/04 03:18:23, apacible wrote: > Rename this to something more descriptive, e.g. hideSinkListForAnimation_. > Otherwise this seems like an override for showing the sink list at first glance. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:223: heightAdjustPlayer_: { On 2016/03/04 03:18:22, apacible wrote: > Rename this to something more descriptive, e.g. heightAdjustmentPlayer_. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:229: * Whether the search list is currently hidden. This is closely related to On 2016/03/04 03:18:22, apacible wrote: > Explain how this is closely related. Why do we need both? > > For search and animations, we're keeping track of a number of properties, such > as: > - isSearchListHidden_ > - isUserSearching_ > - forceSinkListHidden_ > - MediaRouterView.FILTER > ... etc > > Can we consolidate some of these to make it simpler? For example, instead of > having |isUserSearching_|, set |currentView_| as FILTER. That also removes one > of the observer calls. Simplifying the properties in general will also make the > states less fragile. |isUserSearching_| is indeed less loaded now so that has been eliminated in favor of setting |currentView_|. The other two hidden variables are still necessary. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:305: player_: { On 2016/03/04 03:18:23, apacible wrote: > Rename this to something more descriptive. filterTransitionPlayer_ ? Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1115: * @param {number} speed Optional rate of movement in pixels per second. On 2016/03/04 03:18:22, apacible wrote: > |speed| isn't specified in any of the function calls. Will this be needed in a > later patch? If not, remove it. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1279: var fromHeight = list.offsetHeight + search.offsetHeight; On 2016/03/04 03:18:23, apacible wrote: > nit: rename to initialHeight and finalHeight. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1288: var listEffect = new KeyframeEffect(list, On 2016/03/04 03:18:23, apacible wrote: > Add comments on what the expected behavior in this GroupEffect is. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1304: var finalizeAnimation = function() { On 2016/03/04 03:18:23, apacible wrote: > We talked offline on how this is queued up in situations such as when the user > is still searching while the animation is live. Add some comments here > summarizing this. Done. I also removed the branch and just left the |cancel()| call completely to |putSearchAtTop_|. I think it makes things a little cleaner. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1341: var fromHeight = search.offsetHeight + resultsContainer.offsetHeight; On 2016/03/04 03:18:23, apacible wrote: > initialHeight/finalHeight here as well. Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1552: var toHeight = this.computeTotalSearchHeight_(noMatches, results, search, On 2016/03/04 03:18:23, apacible wrote: > finalHeight here as well Done. https://codereview.chromium.org/1754713005/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1819: this.isUserSearching_ = false; On 2016/03/04 03:18:22, apacible wrote: > Move into if block. Done.
Looks great, just nits. Have you run this through the closure compiler? I'll patch this into a mac build for a sanity check. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:25: * @type {Promise} nit: @type {?Promise} https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:30: value: function() { nit: value: null, https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:191: * @type {boolean} @private {boolean} https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:220: * @type {webAnimationsNext.Animation} @private: {?webAnimationsNext.Animation} https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:230: * @type {boolean} @private {boolean} https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:291: * @type {webAnimationsNext.Animation} @private {?webAnimationsNext.Animation} https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:503: // TODO(btolsch): Prevent window re-focus from entering search mode. Is there a crbug for this? https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:964: * @param {boolean} forceHiddenForAnimation Force the sink list to be hidden. nit: "Whether to hide the sink list for an animation." https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1106: durationFromSpeed_: function(distance) { nit: getAnimationDuration_() or computeAnimationDuration_(). The current function name implies speed may be variable. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1107: speed = 1; // 1000 pixels per second nit: |speed| isn't declared, but it's also not needed. How about something like: durationFunctionName_: function(distance) { // The duration of the animation can be found by abs(distance)/speed, where // speed is fixed at 1000 pixels per second, or 1. return Math.abs(distance); } https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1319: moveSearchToBottom_: function(stuff) { Remove stuff? https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1670: this.filterSinks_(this.searchInputText_ || ''); |searchInputText_| is never set to null. If there was no input text, it would be an empty string anyway.
If fixed the nits and added declarations to externs.js to get closure compilation working. PTAL and let me know how the mac build goes. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:25: * @type {Promise} On 2016/03/07 19:29:56, apacible wrote: > nit: @type {?Promise} Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:30: value: function() { On 2016/03/07 19:29:56, apacible wrote: > nit: value: null, Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:191: * @type {boolean} On 2016/03/07 19:29:56, apacible wrote: > @private {boolean} Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:220: * @type {webAnimationsNext.Animation} On 2016/03/07 19:29:56, apacible wrote: > @private: {?webAnimationsNext.Animation} Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:230: * @type {boolean} On 2016/03/07 19:29:56, apacible wrote: > @private {boolean} Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:291: * @type {webAnimationsNext.Animation} On 2016/03/07 19:29:56, apacible wrote: > @private {?webAnimationsNext.Animation} Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:503: // TODO(btolsch): Prevent window re-focus from entering search mode. On 2016/03/07 19:29:56, apacible wrote: > Is there a crbug for this? No but I already have a patch. I will create one and point that issue to it. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:964: * @param {boolean} forceHiddenForAnimation Force the sink list to be hidden. On 2016/03/07 19:29:56, apacible wrote: > nit: "Whether to hide the sink list for an animation." Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1106: durationFromSpeed_: function(distance) { On 2016/03/07 19:29:56, apacible wrote: > nit: getAnimationDuration_() or computeAnimationDuration_(). > > The current function name implies speed may be variable. Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1107: speed = 1; // 1000 pixels per second On 2016/03/07 19:29:56, apacible wrote: > nit: |speed| isn't declared, but it's also not needed. > > How about something like: > > durationFunctionName_: function(distance) { > // The duration of the animation can be found by abs(distance)/speed, where > // speed is fixed at 1000 pixels per second, or 1. > return Math.abs(distance); > } Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1319: moveSearchToBottom_: function(stuff) { On 2016/03/07 19:29:56, apacible wrote: > Remove stuff? Done. https://codereview.chromium.org/1754713005/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1670: this.filterSinks_(this.searchInputText_ || ''); On 2016/03/07 19:29:56, apacible wrote: > |searchInputText_| is never set to null. If there was no input text, it would be > an empty string anyway. I think it might be set to null somewhere in the property binding setup from the html. Whatever the reason though, it is null when inside this function when first opening the dialog which necessitates the logical OR here.
lgtm It looks fine on osx right now, but worth doing more thorough testing after autoresizing goes in. https://codereview.chromium.org/1754713005/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1754713005/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:191: #sink-list-view { nit: alphabetize. https://codereview.chromium.org/1754713005/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:25: * @type {?Promise} @private {?Promise} https://codereview.chromium.org/1754713005/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:191: hideSinkListForAnimation_: { nit: here and below - alphabetize properties. https://codereview.chromium.org/1754713005/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1547: this.computeAnimationDuration_(finalHeight - view.offsetHeight), nit: indent another 2 spaces.
The idea and effect are essentially the same but I have simplified the animation queueing as well as other small changes since the rebase. The flaky browser tests may still need to be fixed before landing this, but PTAL in the meantime.
lgtm once flaky test issues are resolved. https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html (right): https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:109: hidden$="[[hideSinkListForAnimation_]]"> Replace isUserSearching_ with hideSinkListForAnimation_. That way, we only need to check whether to hide this once, rather than in both <template> and <paper-menu>. https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:101: * @private {Animation} Here and below: {?Animation} ? https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:103: filterTransitionPlayer_: { nitty nit: alphabetize the new properties here and below. https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1020: (noMatches.hasAttribute('hidden')) ? results.offsetHeight : nit: Fix indentation.
https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html (right): https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:109: hidden$="[[hideSinkListForAnimation_]]"> On 2016/04/27 23:16:08, apacible wrote: > Replace isUserSearching_ with hideSinkListForAnimation_. > > That way, we only need to check whether to hide this once, rather than in both > <template> and <paper-menu>. This was intentional since the animation needs properties of #sink-list while it may be hidden. It will not work as-is after moving hideSinkListForAnimation_. I would also have to split up the animation functions with setTimeout() in the middle to allow for the stamping to occur. Also, I feel that stamping/removing the list during view transitions isn't necessarily a good thing. If you do feel strongly that it should be moved, I can try to amend the animation code to cope with it. https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:101: * @private {Animation} On 2016/04/27 23:16:08, apacible wrote: > Here and below: {?Animation} ? Object implies nullable. I thought usage was mixed here but after looking again I think these uses are only mine. I think mixed usage is only in the extension. As such, I changed mine to be consistent with this file and use the '?'. https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:103: filterTransitionPlayer_: { On 2016/04/27 23:16:08, apacible wrote: > nitty nit: alphabetize the new properties here and below. This one seems right, but fixed the properties below. https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1020: (noMatches.hasAttribute('hidden')) ? results.offsetHeight : On 2016/04/27 23:16:08, apacible wrote: > nit: Fix indentation. This admittedly weird call is what clang-format gave me. I pulled the conditional out into a separate variable instead.
https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html (right): https://codereview.chromium.org/1754713005/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:109: hidden$="[[hideSinkListForAnimation_]]"> On 2016/04/28 00:19:53, btolsch wrote: > On 2016/04/27 23:16:08, apacible wrote: > > Replace isUserSearching_ with hideSinkListForAnimation_. > > > > That way, we only need to check whether to hide this once, rather than in both > > <template> and <paper-menu>. > > This was intentional since the animation needs properties of #sink-list while it > may be hidden. It will not work as-is after moving hideSinkListForAnimation_. I > would also have to split up the animation functions with setTimeout() in the > middle to allow for the stamping to occur. Also, I feel that stamping/removing > the list during view transitions isn't necessarily a good thing. > > If you do feel strongly that it should be moved, I can try to amend the > animation code to cope with it. I don't feel strongly on this moving into <template>. This is fine, given all the churn moving it would cause. Thanks!
Description was changed from ========== [Media Router] Animate transition between sink list and filter views. This change adds: - Animated transition between sink list view and filter view. BUG=565696 R=apacible@chromium.org ========== to ========== [Media Router] Animate transition between sink list and filter views. This change adds: - Animated transition between sink list view and filter view. BUG=565696 R=apacible@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by btolsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from apacible@chromium.org Link to the patchset: https://codereview.chromium.org/1754713005/#ps160001 (title: "Rebase for test fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754713005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754713005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by btolsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from apacible@chromium.org Link to the patchset: https://codereview.chromium.org/1754713005/#ps180001 (title: "Rebase for apacible's perf change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754713005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754713005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Animate transition between sink list and filter views. This change adds: - Animated transition between sink list view and filter view. BUG=565696 R=apacible@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Animate transition between sink list and filter views. This change adds: - Animated transition between sink list view and filter view. BUG=565696 R=apacible@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a0482de9d6932c61bd89f6cae43f215cc599f0f3 Cr-Commit-Position: refs/heads/master@{#391422} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a0482de9d6932c61bd89f6cae43f215cc599f0f3 Cr-Commit-Position: refs/heads/master@{#391422} |
