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

Detect non-window element resize #918

Merged
merged 6 commits into from
Dec 29, 2017
Merged

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Dec 26, 2017

@bvaughn can this feature be minor?

@codecov-io
Copy link

codecov-io commented Dec 26, 2017

Codecov Report

Merging #918 into master will decrease coverage by 0.06%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
- Coverage   89.93%   89.87%   -0.07%     
==========================================
  Files          57       57              
  Lines        1620     1630      +10     
==========================================
+ Hits         1457     1465       +8     
- Misses        163      165       +2
Impacted Files Coverage Δ
source/WindowScroller/WindowScroller.js 85.24% <85.71%> (-1.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1936f5b...7f72c44. Read the comment docs.

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I actually think this is fine for a bugfix.

}
if (scrollElement === window) {
window.removeEventListener('resize', this._onResize, false);
} else if (this._detectElementResize && scrollElement) {
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't handle the case where the scrollElement prop changes from window to !window (or vice versa) within the life of the instance. I don't know if that's ever going to actually happen though. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@TrySound
Copy link
Contributor Author

TrySound commented Dec 27, 2017

We can use now or in the future resize-observer-polyfill. It's quite nice and have types. However not sure its strategy is also efficient as current implementation. Chrome promises to enable it by default in 64.

@bvaughn
Copy link
Owner

bvaughn commented Dec 27, 2017 via email

@TrySound TrySound force-pushed the window-scroller-detect-resize branch from 4ccf3d5 to 71c510f Compare December 27, 2017 22:43
@TrySound TrySound force-pushed the window-scroller-detect-resize branch from 71c510f to 7f72c44 Compare December 29, 2017 10:54
@TrySound TrySound merged commit 8849d0b into master Dec 29, 2017
@TrySound TrySound deleted the window-scroller-detect-resize branch December 29, 2017 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants