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

feat(viewport-ruler): add common window resize handler #7113

Merged
merged 2 commits into from
Oct 5, 2017

Conversation

crisbeto
Copy link
Member

Adds the change method to the ViewportRuler, allowing for components to hook up to a common window resize handler.

BREAKING CHANGE: Previously the ScrollDispatcher.scrolled subscription would react both on scroll events and on window resize events. Now it only reacts to scroll events. To react to resize events, subscribe to the ViewportRuler.change() stream.

This is a re-submit of #7055.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 15, 2017
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 15, 2017
@crisbeto crisbeto added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 15, 2017
@mmalerba
Copy link
Contributor

@crisbeto I'm seeing a screenshot test failure where the select's popup winds up misaligned. (I'm assuming due to a window resize in the test and the fact that RepositionScrollStrategy no longer listens for those resize events). This seems like it may be a bigger problem than just the select, all of our popup style components are probably relying on this right now.

@mmalerba mmalerba added presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged and removed action: merge The PR is ready for merge by the caretaker labels Sep 21, 2017
@crisbeto
Copy link
Member Author

I see. I don't think that the resize logic belonged in the scroll strategy in the first place, but I can re-add it in the overlayRef.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Sep 22, 2017
@crisbeto
Copy link
Member Author

@mmalerba I've re-added the resize listener for connected overlays.

@andrewseguin
Copy link
Contributor

Needs rebase

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Sep 30, 2017
@crisbeto crisbeto force-pushed the viewport-ruler-change-stream branch from 3e3d85f to 4cc43f4 Compare October 3, 2017 17:27
@kara kara removed the action: merge The PR is ready for merge by the caretaker label Oct 3, 2017
@kara kara assigned crisbeto and unassigned mmalerba Oct 3, 2017
@kara
Copy link
Contributor

kara commented Oct 3, 2017

@crisbeto Can you fix the linter error when you get a chance?

@crisbeto crisbeto force-pushed the viewport-ruler-change-stream branch from 4cc43f4 to fd0f21f Compare October 3, 2017 19:28
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 3, 2017
@crisbeto
Copy link
Member Author

crisbeto commented Oct 3, 2017

Sorted out the linting issue.

@kara
Copy link
Contributor

kara commented Oct 3, 2017

Gah! Another conflict, sorry @crisbeto

@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Oct 3, 2017
Adds the `change` method to the `ViewportRuler`, allowing for components to hook up to a common window resize handler.

BREAKING CHANGE: Previously the `ScrollDispatcher.scrolled` subscription would react both on scroll events and on window resize events. Now it only reacts to scroll events. To react to resize events, subscribe to the `ViewportRuler.change()` stream.
@crisbeto crisbeto force-pushed the viewport-ruler-change-stream branch from fd0f21f to 6af4cd8 Compare October 4, 2017 17:04
@crisbeto
Copy link
Member Author

crisbeto commented Oct 4, 2017

Rebased.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Oct 4, 2017
@kara
Copy link
Contributor

kara commented Oct 4, 2017

@crisbeto Check out the test failures? Probably rebase problem?

@kara kara removed the action: merge The PR is ready for merge by the caretaker label Oct 4, 2017
@crisbeto crisbeto force-pushed the viewport-ruler-change-stream branch from 6af4cd8 to c9a4f93 Compare October 4, 2017 19:09
@crisbeto
Copy link
Member Author

crisbeto commented Oct 4, 2017

Seems like something that got introduced from master. Fixed.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 4, 2017
@kara kara merged commit 3b0915a into angular:master Oct 5, 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 7, 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 P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants