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

Support display:none iframes in flexgap test #2593

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chris13524
Copy link
Contributor

Fixes #2590

@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #2593 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2593   +/-   ##
=======================================
  Coverage   95.15%   95.15%           
=======================================
  Files           5        5           
  Lines         165      165           
=======================================
  Hits          157      157           
  Misses          8        8           

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 55f3f36...737279a. Read the comment docs.

@Markel
Copy link
Contributor

Markel commented Aug 25, 2020

Lgtm 😄 as long as making it async doesn't make it backwards incompatible as we discussed in #2590 (which I doubt it but better to be sure than sorry).

The only thing is that inside the else comment (line 43) it would be nice to explain that the test doesn't work if we are inside a iframe:none and that is why we wait. Simply adding a #2590 will probably be enough for people in the future to find out the reasoning.

@chris13524
Copy link
Contributor Author

@Markel good idea, I just pushed a comment linking to the issue

@chris13524
Copy link
Contributor Author

chris13524 commented Aug 25, 2020

Re: backwards compatibility. I think the only way this would break compatibility is if they use Modernizr.flexgap during page load. Rather than returning false it would be undefined.

To potentially avoid this, we could adapt the test to be sync under normal circumstances and only be async if it detects it is inside a display:none iframe. However, this would still have the issue of Modernizr.flexgap not working under the async case. Also not sure if it is acceptable to have the async/sync nature switch out depending on runtime conditions.

Or if there is some way to remove my setTimeout() call, that would be ideal as I think it would support using Modernizr.flexgap during page load, even though it is technically an async test.

@Markel
Copy link
Contributor

Markel commented Aug 26, 2020

I don't really know how to contribute in here, I don't really know. Maybe @rejas knows better :)

@Markel
Copy link
Contributor

Markel commented Oct 3, 2020

Maybe we should get back to this? 👀

@rejas
Copy link
Member

rejas commented Oct 3, 2020

definitly. maybe @patrickkettner can take a look to :-) ?

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.

Flex gap detection does not work when inside display:none iframe
3 participants