Skip to content
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

Merged
merged 5 commits into from
Jan 12, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Dec 8, 2016

Fixes the select being transparent on browsers that allow overscrolling (mostly mobile).

Here's what it looks like before the fix:
img_1189

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 8, 2016
@jelbourn jelbourn requested a review from kara December 8, 2016 21:41
@@ -34,7 +34,7 @@
}
}

.md-select-content {
.md-select-panel {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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?

@crisbeto crisbeto force-pushed the select-transparent-background branch from 5f59ebf to 7dc4767 Compare December 12, 2016 20:20
Copy link
Contributor

@kara kara left a 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' : ''">
Copy link
Contributor

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;
Copy link
Contributor

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?

@crisbeto crisbeto force-pushed the select-transparent-background branch from 7dc4767 to c597771 Compare December 13, 2016 20:50
@crisbeto
Copy link
Member Author

Updated again @kara. Good spot with resetting of the value, it would've gotten broken if it went through.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto force-pushed the select-transparent-background branch from c597771 to 7c0dd1c Compare December 20, 2016 08:56
@kara
Copy link
Contributor

kara commented Jan 12, 2017

@crisbeto Is this ready to go?

@crisbeto
Copy link
Member Author

It is @kara

@kara kara added the action: merge The PR is ready for merge by the caretaker label Jan 12, 2017
@tinayuangao tinayuangao merged commit d9b2d85 into angular:master Jan 12, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants