-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(select): transparent background when overscrolling #2117
fix(select): transparent background when overscrolling #2117
Conversation
@@ -34,7 +34,7 @@ | |||
} | |||
} | |||
|
|||
.md-select-content { | |||
.md-select-panel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto This will change the animation and take it off spec. Any other way to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The background has to be on the scrollable container. I can try adding it after all animations are done, otherwise the alternative is to make the md-select-content
scrollable, but it might break more stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, try adding it after the panel animation finishes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time trying to get it to work, but the only way ended up being kind of ugly. @kara do you know whether this can be simplified (ignore the colors and timings, I've exaggerated them for easier testing):
export const transformPanel: AnimationEntryMetadata = trigger('transformPanel', [
state('showing', style({
opacity: 1,
minWidth: 'calc(100% + 32px)',
transform: `translate3d(0,0,0) scaleY(1)`,
backgroundColor: '#f00'
})),
transition('void => *', [
style({
opacity: 0,
minWidth: '100%',
transform: `translate3d(0, 0, 0) scaleY(0)`
}),
animate(`1500ms cubic-bezier(0.25, 0.8, 0.25, 1)`, style({
opacity: 1,
minWidth: 'calc(100% + 32px)',
transform: `translate3d(0,0,0) scaleY(1)`
})),
animate('0ms 1500ms linear', style({
backgroundColor: '#f00'
}))
]),
transition('* => void', [
animate('2500ms 1000ms linear', style({opacity: 0}))
])
]);
The change is that the styles from the showing
state have to be repeated below as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto Given that the color should come in immediately after the animation (rather than animating in), it might be easier to do this outside of the animation definition. Have you tried adding something to the existing "done" hook for the animation? There is an _onPanelDone method that is called when the animation finishes. Try toggling a style or class binding at that point instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason was that I wanted to keep it together with the animation logic since it's animation-related. I've changed it, can you take another look @kara?
5f59ebf
to
7dc4767
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this approach better. Can you add a test for the correct application of the class?
backdropClass="md-overlay-transparent-backdrop" [positions]="_positions" [minWidth]="_triggerWidth" | ||
[offsetY]="_offsetY" [offsetX]="_offsetX" (attach)="_setScrollTop()"> | ||
<div class="md-select-panel" [@transformPanel]="'showing'" (@transformPanel.done)="_onPanelDone()" | ||
(keydown)="_keyManager.onKeydown($event)" [style.transformOrigin]="_transformOrigin"> | ||
(keydown)="_keyManager.onKeydown($event)" [style.transformOrigin]="_transformOrigin" | ||
[ngClass]="_panelDoneAnimating ? 'md-select-panel-done-animating' : ''"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using a regular class binding? It might be less verbose.
[class.ng-select-panel-done-animating]="_panelDoneAnimating"
@@ -349,6 +352,8 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr | |||
} else { | |||
this.onClose.emit(); | |||
} | |||
|
|||
this._panelDoneAnimating = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this will only work the first time you open the select. Don't you need to reset this to false when the panel closes?
7dc4767
to
c597771
Compare
Updated again @kara. Good spot with resetting of the value, it would've gotten broken if it went through. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes the select being transparent on browsers that allow overscrolling (mostly mobile).
c597771
to
7c0dd1c
Compare
@crisbeto Is this ready to go? |
It is @kara |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes the select being transparent on browsers that allow overscrolling (mostly mobile).
Here's what it looks like before the fix: