Skip to content

fix(breakpoints): emit only one event for adjacent breakpoint changes. #11007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 23, 2018
Merged

fix(breakpoints): emit only one event for adjacent breakpoint changes. #11007

merged 1 commit into from
Aug 23, 2018

Conversation

@josephperrott
Copy link
Member

@josephperrott josephperrott commented Apr 25, 2018 •
edited
Loading

Fixes #10925
Fixes #10944

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 25, 2018
@josephperrott josephperrott force-pushed the breakpoints branch 2 times, most recently from ccba8d6 to 5095cb5 Compare April 25, 2018 20:01
crisbeto
crisbeto reviewed Apr 25, 2018
View reviewed changes
src/cdk/layout/breakpoints-observer.ts
};
}));
return combineLatest(observables).pipe(
debounceTime(0, asapScheduler),
Copy link
Member

Choose a reason for hiding this comment

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

Should this one run outside the NgZone?

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 don't believe so, we would have to immediately place it back into the NgZone in the next method in the pipe.

@andrewseguin andrewseguin added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Apr 25, 2018
@ngbot
Copy link

ngbot bot commented Apr 25, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure missing required label: "release: *"
    failure status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@josephperrott josephperrott added the target: patch This PR is targeted for the next patch release label Apr 25, 2018
crisbeto
crisbeto approved these changes Apr 25, 2018
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@josephperrott josephperrott force-pushed the breakpoints branch 3 times, most recently from 79c8842 to 7610ff0 Compare April 30, 2018 15:59
@andrewseguin
Copy link
Contributor

Looks like you got real CI errors on this one

@tinayuangao
Copy link
Contributor

@josephperrott Please fix the tests

@josephperrott josephperrott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jul 2, 2018
@CharlBest
Copy link

Is it possible to have this merged into the next release?

@CharlBest
Copy link

@josephperrott do you think this is ready to be merged or are there other problems I don't know about that isn't public on GitHub?

@CharlBest
Copy link

Can this please go into the next release?

@CharlBest CharlBest mentioned this pull request Aug 9, 2018
Merged
@jelbourn jelbourn merged commit 2830a64 into angular:master Aug 23, 2018
@jbojcic1
Copy link

@josephperrott why wasn't this included in 6.4.7 since it has been merged 6 days before that release?

@jbojcic1
Copy link

@josephperrott actually it doesn't even seem to be working. https://stackblitz.com/edit/angular-material2-issue-sc9n2h?file=app/app.component.ts when I try this in chrome I see it still fires twice.

@henry-alakazhang henry-alakazhang mentioned this pull request Oct 28, 2018
Closed
@chandanamadu
Copy link

I' m using angular cdk, material version 7.1.1. But i'm still getting this issue. :(

@gentunian
Copy link

Same here:

 "@angular/material": "^7.1.1",
"@angular/cdk": "^7.1.1",

@TrevorKarjanis TrevorKarjanis mentioned this pull request May 6, 2019
Merged
@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 10, 2019
@josephperrott josephperrott deleted the breakpoints branch March 20, 2020 22:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin andrewseguin approved these changes

Assignees

@andrewseguin andrewseguin

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

breakpoint-observer: incorrectly splitting queries BreakpointObserver.observe emits twice for adjacent breakpoints

10 participants

@josephperrott @andrewseguin @tinayuangao @CharlBest @jbojcic1 @chandanamadu @gentunian @crisbeto @jelbourn @googlebot