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

Added 'omit' option for scrolling parameter #582

Merged
merged 4 commits into from
Oct 28, 2018

Conversation

mryand
Copy link
Contributor

@mryand mryand commented May 11, 2018

Adds #581

@coveralls
Copy link

coveralls commented May 11, 2018

Coverage Status

Coverage decreased (-0.09%) to 93.366% when pulling 035ae53 on carleton:master into f93cead on davidjbradshaw:master.

@davidjbradshaw
Copy link
Owner

davidjbradshaw commented May 11, 2018

Hi,

Rather than add yet another config property , it would be simpler to an ‘omit’ option to the scrolling attribute.

case ‘omit’:
  break;

I think this should achieve the same result with much less code.

@mryand
Copy link
Contributor Author

mryand commented May 11, 2018

I was thinking it should account for the possibility that you'd want to set the style attribute to overflow:hidden; without adding the scrolling attribute.

I'm not sure the style attribute is well supported, though -- my testing suggests that when you use style="overflow:hidden;" without iframe-resizer you still see scrollbars, at least in Chrome. So perhaps that doesn't matter much.

I'll test out the case: 'omit' approach and update the PR.

@mryand mryand changed the title Added useScrollingAttr parameter Added 'omit' option for scrolling parameter May 14, 2018
@mryand
Copy link
Contributor Author

mryand commented May 14, 2018

Ok, I've updated the PR to recognize an omit value for that parameter. I also updated the docs to reflect this change.

@mryand
Copy link
Contributor Author

mryand commented May 14, 2018

I tried to get a test in but clearly I got something wrong. I'm not familiar with qunit (I'm not primarily a js dev if it's not obvious from my code) so it's probably something I missed. On my local machine when I grunt I'm getting an unrelated testing error even without this additional test, so it's hard for me to troubleshoot. Apologies.

@davidjbradshaw
Copy link
Owner

What is the error your seeing?

@davidjbradshaw davidjbradshaw self-requested a review May 15, 2018 18:06
@mryand
Copy link
Contributor Author

mryand commented May 15, 2018

This is the message:

Running "karma:single" (karma) task
14 05 2018 14:41:44.255:INFO [karma]: Karma v0.13.22 server started at http://localhost:9876/
14 05 2018 14:41:44.274:INFO [launcher]: Starting browser Chrome
14 05 2018 14:41:44.287:INFO [launcher]: Starting browser Firefox
14 05 2018 14:41:46.218:INFO [Chrome 66.0.3359 (Mac OS X 10.13.4)]: Connected on socket J0PoZ7z-uclYduVaAAAA with id 37727081
Chrome 66.0.3359 (Mac OS X 10.13.4): Executed 54 of 73 (skipped 1) SUCCESS (0 secs / 0 secs)
Chrome 66.0.3359 (Mac OS X 10.13.4): Executed 72 of 73 (skipped 1) SUCCESS (4.28 secs / 0 secs)
Firefox 60.0.0 (Mac OS X 10.13.0) Get Page info with multiple frames must send pageInfo to second frame FAILED
	Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL. in /Users/mryan/iframe-resizer/node_modules/jasmine-core/lib/jasmine-core/jasmine.js (line 4470)
	attempt/timeoutId<@/Users/mryan/iframe-resizer/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:4470:23
	
	Captured logs:
	  LOG: [iFrameSizer][Nested host page: getPageInfo]', 'Removing iFrame: getPageInfo
	  LOG: [iFrameSizer][Nested host page: getPageInfo]', '--
Chrome 66.0.3359 (Mac OS X 10.13.4): Executed 72 of 73 (skipped 1) DISCONNECTED (14.288 secs / 0 secs)
Firefox 60.0.0 (Mac OS X 10.13.0): Executed 72 of 73 (1 FAILED) (skipped 1) (16.971 secs / 0 secs)

@davidjbradshaw
Copy link
Owner

That is the newer Jasmine tests failing in FireFox. If you go into Karma Config then you can just remove FireFox for now.

@davidjbradshaw davidjbradshaw merged commit 17d5429 into davidjbradshaw:master Oct 28, 2018
@davidjbradshaw
Copy link
Owner

Thanks released in v3.6.3

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.

None yet

3 participants